Bug 84872 - threaded .xlsb import failure
Summary: threaded .xlsb import failure
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
4.4.0.0.alpha0+ Master
Hardware: Other All
: medium normal
Assignee: Michael Meeks
URL:
Whiteboard: BSA target:4.4.0
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-10 12:16 UTC by Caolán McNamara
Modified: 2014-10-13 15:14 UTC (History)
1 user (show)

See Also:
Crash report or crash signature:

Attachments
gdb log (28.17 KB, text/plain)
2014-10-10 12:16 UTC, Caolán McNamara 		Details
/tmp/debug.patch (1.61 KB, patch)
2014-10-10 13:14 UTC, Caolán McNamara 		Details
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Caolán McNamara 2014-10-10 12:16:46 UTC 
Created attachment 107659 [details]
gdb log

crashes in threaded .xlsb imort
Comment 1 Caolán McNamara 2014-10-10 12:20:09 UTC 
Thread #1, frame #13, FormulaParser::importFormula (this=0x2bf0aa0...

Thread #8, frame #23, FormulaParser::importFormula (this=0x2bf0aa...

Two different threads, and two different SheetDataContexts are using the same FormulaParser, which has a FormulaaParserImpl and the two threads are stomping all over the same maTokenIndexes etc
Comment 2 Michael Meeks 2014-10-10 13:04:39 UTC 
Ooh - silly; thanks ! taking that ...
Comment 3 Caolán McNamara 2014-10-10 13:11:07 UTC 
So I don't really see how this can work at all, I mean there is a single FormulaParser for the import and multiple threads sharing it. The FormulaParser has state. Its api claims constness, but its a tissue of lies seeing as it forwards everything to a non-const pointer to a pImpl which is riddled with state as far as I can see.

Presumably locking calls to the shared SheetDataContext mrFormulaParser defeats the purpose, so have a separate formula parser per SheetDataContext ?

Document is fdo60899-1.xlsb FWIW
Comment 4 Michael Meeks 2014-10-10 13:12:58 UTC 
The xlsb in question is from bug#60899 https://bugs.freedesktop.org/attachment.cgi?id=74875
Comment 5 Michael Meeks 2014-10-10 13:13:41 UTC 
Riight - having separate FormulaParsers seems an attractive option; hmm. I'll look at it over the next days.
Comment 6 Caolán McNamara 2014-10-10 13:14:41 UTC 
Created attachment 107661 [details]
/tmp/debug.patch

Heres how I generated the bt. My debugging hack. Because there is only one FormulaParserImpl then this works, obviously aHack would need to be a member of FormulaParserImpl otherwise. About 1 or 2 times out of 5 the above clean bt is generated, otherwise its somewhere else.
Comment 8 Michael Meeks 2014-10-13 11:38:24 UTC 
Oh, wow - thanks for that - I was going to look at it on the plane.

My hope was that the amount of 'real' parsing we do should be small in most cases - the predictive reverse printing & comparison being adequate for most of the big sheet cases; such that we could (should) have protected all the deeper parsing code with the solar mutex - but - of course, if we have a better, faster solution like this - that's great =)

Thanks !
Comment 9 Caolán McNamara 2014-10-13 15:14:40 UTC 
Its a simple solution anyway. No idea if its the fastest one, but hard to imagine micro locking a shared formulaparser would be better, but maybe it is for all I know