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

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