Bug 93994 - IMPRESS: ODP not opening anymore in Windows
Summary: IMPRESS: ODP not opening anymore in Windows
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Impress (show other bugs)
Version:
(earliest affected)
4.1.0.4 release
Hardware: All Windows (All)
: high major
Assignee: Armin Le Grand (allotropia)
URL:
Whiteboard: odf target:5.2.0 target:5.1.4
Keywords: notBibisectable, regression
Depends on:
Blocks:
 
Reported: 2015-09-07 14:41 UTC by Gorka Navarrete
Modified: 2016-10-25 19:02 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:


Attachments
Document that crashes when trying to open in windows. (131.43 KB, application/vnd.oasis.opendocument.presentation)
2015-09-07 14:41 UTC, Gorka Navarrete
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Gorka Navarrete 2015-09-07 14:41:48 UTC
Created attachment 118500 [details]
Document that crashes when trying to open in windows.

The attached document does not open in LO for windows. The progress bar advances up to 3/4 and LO crashes without a warning.

It does open in a Linux / Ubuntu computer (a more powerful pc though).

It does open in Windows when spitted in two equal parts.
Comment 1 Timur 2015-09-07 16:05:19 UTC
Repro in Windows with 5.1+. Last worked with 4.0.5. 
I suspected to Bug 70783 because of the similar problem and the same version but it behaves differently.
Comment 2 Timur 2015-09-07 16:35:11 UTC
Probably a duplicate of Bug 78985.
Comment 3 Gorka Navarrete 2015-09-07 18:17:45 UTC
(In reply to Timur from comment #2)
> Probably a duplicate of Bug 78985.

Can someone check if the file in Bug 78985 opens up in Windows when split in half? That is the case with the file attached in this Bug, and may or may not mean that there is something specific to this bug. I have no easy access to Windows to try it myself. :(
Comment 4 Timur 2015-10-13 16:12:14 UTC
I guess I was wrong, WinDBG errors are different. 
Could you make a backtrace and even a valgrind with https://wiki.documentfoundation.org/QA/BugReport/Debug_Information?
Comment 5 Armin Le Grand (allotropia) 2015-11-12 14:46:18 UTC
Tried on Win7, 32bit LO  5.1.0.0.alpha1+.
Happens as described. Looking deeper (SD_XML_READERROR in SdXMLFilter::Import)...
Comment 6 Armin Le Grand (allotropia) 2015-11-12 15:52:32 UTC
Looking in the file says there is a presentation with 16 pages. Suppressing the load error shows 12 of them.
Got further. During import many CustomShapes get imported. Each one seems to create an outliner for import, so in SdrOutlinerCache::createOutliner the number of ActiveOutliners is already 4904. The new to be created one also creates a VirtualDevice::ImplInitVirDev which fails. Every outliner seems to do that, so there is quite a number of VirtualDevices incarnated. Checking why each import of a CustomShape creates an own outliner and seems not to free it...
Comment 7 Armin Le Grand (allotropia) 2015-11-12 15:53:18 UTC
Ah, point to break is VirtualDevice::ImplInitVirDev at the throw statement...
Comment 8 Armin Le Grand (allotropia) 2015-11-13 11:56:46 UTC
Looks like every SvxCustomShape is holding a SvxShapeText by being derived by it. This holds the on-demand created EditEngine (and the VirtualDevice as RefDevice), too. All EditEngines get created at load time due to the text import and stay at the SvxCustomShape incarnation.
Checked that this is correct ans these are freed at document close. Investigating to find a mechanism how to not hold that EditEngine for all objects after load time.
Comment 9 Armin Le Grand (allotropia) 2015-11-13 14:45:20 UTC
It is hard to find a point in the program flow to reset the on-demand created outliners. The on-demand creation and cacing (SdrOutlinerCache) is done behind the UNO API while the load is in xmloff on the other side.
Detected a place where the on-demand outliner (and the VirtualDevice) held at SvxTextEditSourceImpl can be flushed. To detect, it triggers when
- there is an outliner
- there is a model
- the model is locked (at load time)
- only one object (1 == maRefCount) uses the outliner (the SdrObject/SvxObject)
- and the object is a SdrObjCustomShape
This works and reduces the number of Outliners (and VirtualDevices) used when loading the document from ca. 4200 (until it crashes) to one which is even cached by the SdrOutlinerCache. Doing some more tests...
Comment 10 Armin Le Grand (allotropia) 2015-11-13 14:54:25 UTC
Added a warning to SdrOutlinerCache when more than a decent number of Outliners is used at the same time. This is usually a hint that something goes wrong. It will also trigger if this fix gets broken somehow.
Comment 11 Armin Le Grand (allotropia) 2015-11-16 08:42:01 UTC
1st try not sufficient, problems in saving CustomShapes with text content. Revising solution, will have to use an internal UNO API slot to trigger the event.
Comment 12 Armin Le Grand (allotropia) 2015-11-16 09:00:12 UTC
Works close to expected, there are still 303 outliners used as maximum count. Looks as if for each <text/> statement in the source file a SvxUnoTextCursor gets created, so not only the CustomShapes use one. This is done for empty texts when creating SdXMLCustomShapeContext::CreateChildContext, not sure if this is really needed. Checking...
Comment 13 Armin Le Grand (allotropia) 2015-11-16 09:24:57 UTC
SdXMLShapeContext::CreateChildContext creates a new createTextCursor() in mxCursor, this happens a lot of times. mxOldCursor from current text import is saved and restored, so at restore time mxCursor should be reset, too. This *should* cleanup the references and lead to deletion/freeing of the outliner. Checking why this currently blocks and holds the outliner...
Comment 14 Armin Le Grand (allotropia) 2015-11-16 11:49:07 UTC
Found that XMLShapeImportHelper::addGluePointMapping holds references to the shapes as long as the page exists. This is due to

maShapeGluePointsMap[xShape][nSourceId] = nDestinnationId

and thus there are sequences during load which use about 303 existing SvxCustomShapes. Checking if this is needed...
Comment 15 Armin Le Grand (allotropia) 2015-11-16 12:45:46 UTC
Gluepoint stuff seems not to be needed, but it is just too unsafe to do changes here, first the area wold have to be much more inspected. For now, live with 303 Outliners at the same time, warn with really many of them (1000), both relative to the count of 4200 needed for crashing.
Doing more tests with changed code, im/export, use gluepoints, ...
Comment 16 Armin Le Grand (allotropia) 2015-11-16 13:13:36 UTC
Solution added to gerrit.
Comment 17 Armin Le Grand (allotropia) 2015-11-16 14:39:55 UTC
Solution does not work in the proposed way, gerrit found a not-working UnitTest and is right (shows that gerrit, the build there and UnitTests work). Removing the UNO API implementation is too radical. This destroys the GluePoint and connector handling which is executed when a whole page is loaded. Checking if removal of the Outliner is possible, too.
Comment 18 Armin Le Grand (allotropia) 2015-11-16 16:55:46 UTC
Added as a standard mechanism a SdrOutlinerOwner as strict virtual base class containing a tryToReleaseSdrOutliner-callback. The Outliner creator registers, and the SdrOutlinerCache is adapted to try to use this request beginning with a defined number of parallell existing outliners (20 currently). This way only the on-demand outliner is removed (and reused by the SdrOutlinerCache), not the UNO API implementation object.
Working on some details, first tests look good.
Comment 19 Armin Le Grand (allotropia) 2015-11-17 09:36:49 UTC
Looks good, added version to gerrit. Build failed on mac and Linux, both want a virtual destructor for the strict virtual class SdrOutlinerOwner (could the compiler do that please?), adding that...
Comment 20 Armin Le Grand (allotropia) 2015-11-18 14:03:43 UTC
Does not work with one testcase and gerrit is right, it is too dangerous to remove the outliners from the UNO API text import context. Tried different other things, but nothing is convenient.
Went back to 1st idea and looked deeper at the connector stuff which failed in the 1st version. Changed to flush UNO API implementations of SdrObjCustomShape at end of page load (SdXMLDrawPageContext::EndElement()). That allows import since the connectors will be already processed.
The file is not corrupt at all. Question is if import works on 64bit systems. Testing that...
Comment 21 Armin Le Grand (allotropia) 2015-11-18 14:26:41 UTC
Works on 64bit, so only std win is in focus, and there is a 64bit version available now. Also checked if saving is possible in modified 32bit version, too - this could potentially run in the same ressource problem, but does not. I will use gerrit to checl the unit tests on all systems.
Comment 22 Armin Le Grand (allotropia) 2015-11-19 06:55:44 UTC
Build worked now, done so far.
Comment 23 Armin Le Grand (allotropia) 2015-12-07 12:50:40 UTC
After internal discussion agreed to add without restrictions to 32bit to better detect evtl. problems coming along with this change. Preparing updated change and adding to gerrit.
Comment 24 Robinson Tryon (qubit) 2015-12-10 01:26:36 UTC Comment hidden (obsolete)
Comment 25 Commit Notification 2015-12-11 17:11:40 UTC
Armin Le Grand committed a patch related to this issue.
It has been pushed to "master":

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

Resolves: tdf#93994 flush API objects at load time

It will be available in 5.2.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 26 Timur 2015-12-21 17:12:45 UTC
File opens now. Should it be marked as fixed? Please backport.
Comment 27 Timur 2016-02-03 18:23:18 UTC
I guess it is.
Comment 28 Commit Notification 2016-05-11 12:56:42 UTC
Armin Le Grand committed a patch related to this issue.
It has been pushed to "libreoffice-5-1":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=67755e4a3b0634d123ad68b107381bf90b6d6487&h=libreoffice-5-1

Resolves: tdf#93994 flush API objects at load time

It will be available in 5.1.4.

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.