Bug 114479

Summary: Crash when loading an ods file when threading is enabled
Product: LibreOffice Reporter: Dennis Francis <dennisfrancis.in>
Component: CalcAssignee: Not Assigned <libreoffice-bugs>
Status: RESOLVED FIXED    
Severity: normal CC: xiscofauli
Priority: medium    
Version: 6.1.0.0.alpha0+   
Hardware: All   
OS: All   
Whiteboard: target:6.1.0 target:6.0.0.1
Crash report or crash signature: Regression By:
Bug Depends on:    
Bug Blocks: 114159    
Attachments: assert failure at ScDocument::GetFormatTable() (backtrace)
TextSearch object sharing b/w threads (backtrace)

Description Dennis Francis 2017-12-15 05:32:03 UTC 
Description:
The LO build from master crashes on loading https://bugs.documentfoundation.org/attachment.cgi?id=120128 when formula group threading is enabled in calc.

Steps to Reproduce:
1. Open the file https://bugs.documentfoundation.org/attachment.cgi?id=120128 with calc with threading enabled.

Actual Results:  
Calc crashes on loading

Expected Results:
No crash


Reproducible: Always


User Profile Reset: No



Additional Info:
There are multiple issues with this document from a threading perspective :

1) assert() fails inside ScDocument::GetFormatTable()

2) A SUMIF() implicit range problem.

The offending SUMIF here in the document is :

=SUMIF($ResPer.$G$5:$G$593,"^"&D272,ResPer.$R$5)+SUMIF($ResPer.$G$5:$G$
593,"^"&D272,ResPer.$S$5)+SUMIF($ResPer.$G$5:$G$593,"^"&D272,ResPer.$AD
$5)+SUMIF($ResPer.$G$5:$G$593,"^"&D272,ResPer.$AE$5)


Notice that the third arg to each SUMIF is just a single ref, but
during evaluation, it will be implicitly expanded to the length/size of
the first argument doubleref.

The ScDependantsCalculator::Doit() simply pre-interprets all singlerefs
and doublerefs in the token array before any threading begins, hence
misses this special case requirement of SUMIF.

3) Apart from this, we need to blacklist MATCH() opcode for threading as this one leads to calling of the slot machine like VLOOKUP does.

4) Finally there is the problem of sharing the TextSearch object
between threads without any mutex protection that happens on SUMIF(S), COUNTIF(S) or anything that involves a call to ScInterpreter::IterateParametersIfs().


User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.94 Safari/537.36
Comment 1 Dennis Francis 2017-12-15 05:34:43 UTC 
Created attachment 138455 [details]
assert failure at ScDocument::GetFormatTable() (backtrace)
Comment 2 Dennis Francis 2017-12-15 05:35:58 UTC 
Created attachment 138456 [details]
TextSearch object sharing b/w threads (backtrace)
Comment 3 Commit Notification 2017-12-15 09:36:18 UTC 
Dennis Francis committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=2ba36f21bad74cbbd68611afddaf9bbdc8ffda50

tdf#114479 : Blacklist MATCH() for threading

It will be available in 6.1.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 4 Commit Notification 2017-12-15 11:52:56 UTC 
Dennis Francis committed a patch related to this issue.
It has been pushed to "libreoffice-6-0":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=574b06d36e3f0df1dfa1029e8d33473237807dcb&h=libreoffice-6-0

tdf#114479 : Blacklist MATCH() for threading

It will be available in 6.0.0.1.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 5 Commit Notification 2017-12-19 13:10:42 UTC 
Dennis Francis committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=ced2f083483e2d60c077177b8a359d310e0976c3

tdf#114479 : Use the SvNumberFormatter from ScInterpreterContext

It will be available in 6.1.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 6 Commit Notification 2017-12-19 14:56:29 UTC 
Dennis Francis committed a patch related to this issue.
It has been pushed to "libreoffice-6-0":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=7a4bbce22fff8155825447eb0e28c5b09d659fcc&h=libreoffice-6-0

tdf#114479 : Use the SvNumberFormatter from ScInterpreterContext

It will be available in 6.0.0.1.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 7 Xisco Faulí 2018-01-19 10:04:43 UTC 
A polite ping to Dennis Francis: is this bug fixed? if so, could you
please close it as RESOLVED FIXED ? Thanks
Comment 8 Dennis Francis 2018-01-19 10:15:46 UTC 
(In reply to Xisco Faulí from comment #7)
> A polite ping to Dennis Francis: is this bug fixed? if so, could you
> please close it as RESOLVED FIXED ? Thanks

This is not yet fixed completely, 2 out of the 4 issues with this sheet are still pending. I'm putting this back to NEW in case someone else has time to fix this sooner than me. If not I'll get back to this in a couple of weeks. Thanks.
Comment 9 Commit Notification 2018-04-28 11:34:56 UTC 
Dennis Francis committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=67b1c26c27590678ece7bcef763433aedd0b164d

tdf#114479: compute implicit sum ranges for ocSumIf,ocAverageIf...

It will be available in 6.1.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 10 Xisco Faulí 2018-05-29 09:35:40 UTC 
A polite ping to Dennis Francis:
Is this bug fixed? if so, could you please close it as RESOLVED FIXED ? Otherwise, Could you please explain what's missing?
Thanks
Comment 11 Dennis Francis 2018-05-29 10:28:59 UTC 
(In reply to Xisco Faulí from comment #10)
> A polite ping to Dennis Francis:
> Is this bug fixed? if so, could you please close it as RESOLVED FIXED ?
> Otherwise, Could you please explain what's missing?
> Thanks

The 4th issue mentioned in comment#1 is yet to be fixed. All others are done.
Comment 12 Dennis Francis 2018-05-29 10:30:26 UTC 
(In reply to Dennis Francis from comment #11)
> (In reply to Xisco Faulí from comment #10)
> > A polite ping to Dennis Francis:
> > Is this bug fixed? if so, could you please close it as RESOLVED FIXED ?
> > Otherwise, Could you please explain what's missing?
> > Thanks
> 
> The 4th issue mentioned in comment#1 is yet to be fixed. All others are done.

Actually it is mentioned in the description, not comment#1.
Comment 13 Dennis Francis 2018-06-21 11:09:18 UTC 
I can't reproduce the last issue (#4) anymore. Marking as resolved fixed for now.