Apache OpenOffice (AOO) Bugzilla – Issue 124073
endless recursion between AnimatedExtractingProcessor2D and BaseProcessor2D
Last modified: 2017-05-20 10:35:09 UTC
A crash was observed when running a presentation imported from a PPT that is caused by an endless recursion: BaseProcessor2D::process() is called by AnimatedExtractingProcessor2D::processBasePrimitive2D() and vice versa unless the stack runs out at last. Here are the two stack frames, the source file names and the line numbers seen in the recursion. AnimatedExtractingProcessor2D::processBasePrimitive2D(drawinglayer::primitive2d::BasePrimitive2D const&) + 511 at viewobjectcontact.cxx:152 drawinglayer::processor2d::BaseProcessor2D::process(this=0x00007fff5fbfa1f0, rSource=0x00007fff5fbfa098) + 298 at baseprocessor2d.cxx:71
The bugdoc is available at https://svn.apache.org/repos/asf/incubator/ooo/trunk/test/testgui/data/pvt/complex_100p.ppt and the crash was observed when playing slide 21
Taking a look...
No crash in win version, the given methods seem to be 64bit, so maybe linux or mac. Slide 21 looks good on windows, in the slide preview, the slide itself and when presenting.
Content on slide 21 is a WinMetaFile, internally a GdiMetafile. It contains no animations (the AnimatedExtractingProcessor2D is the instance which extracts animated text, the old blink/forthback/... stuff). I see no way how AnimatedExtractingProcessor2D could produce a recursion, it does not create objects with own ViewObjectContact. Could you please add the stack?
The crash could also be in slide 22 (i.e. the last slide one sees is slide 21).
Created attachment 82351 [details] stack of the crash always alternating between viewobjectcontact.cxx:152 and baseprocessor2d.cxx:71 until crash
Created attachment 82352 [details] stack similar to the reported crash a similar stack from another (confidential) bugdoc where the endless recursion comes from alternating between vclmetafileprocessor2d.cxx:2140 and baseprocessor2d.cxx:71
Comment on attachment 82352 [details] stack similar to the reported crash always alternating between vclmetafileprocessor2d.cxx:2140 and baseprocessor2d.cxx:71 instead
Since the crash does not happen here, could you please reduce the bugdoc to a single slide to make sure where it happens. Just use MSO and remove all other slides (I did that for slide 21). Thanks!
Another comment: When does it crash? The whole primitive creation including all slides and usage of AnimatedExtractingProcessor2D already happens directly after load to create the slide previews, so does it already crash directly after load or when you go to that slide (and if, in slideshow or in edit view)?
Created attachment 82364 [details] reduced bugdoc With the reduced bugdoc the crash now happens immediately.
Still no crash on win. Again stepped through the usage of AnimatedExtractingProcessor2D in ViewObjectContact::checkForPrimitive2DAnimations, could not find even a possible scenario for a recursion. To make that happen the decomposition of a primitive of type A would have to create a sequence of primitives containing at least one primitive of type A again or one that decomposes itself containing one of type A. If that would be the case, not a single repaint would work since the same sequence of primitives is used for repaint than for checking if animated text is contained. Please check which types of primitives are contained in the endless loop (best is to break in BaseProcessor2D::process line 71 and look at pBasePrimitive, this is already the implementation class). This is not allowed to be the same type in the recursive stack frame. If this happens, check how this is possible. I can find no theoretical possibility for this. HTH!
@Herbert: have you seen #124005#, may also be Mac64bit specific with VCL controls ?!? Just a thought...
The object that is involved in the endless recursion has the type GraphicPrimitive2D. I saw something suspicious in the line graphicprimitive2d.cxx:95 Graphic aTransformedGraphic(rGraphicObject.GetGraphic()); where an object derived from unographic::Graphic (that has virtual methods) gets "sliced" to its base class object. When that sliced object is used for create2DDecompositionOfGraphic() then there could be all kinds of type-deduction problems. Is it possible to avoid the slicing by e.g. using references or pointers?
I just saw that it is not a unographic::Graphic but a graph.hxx's Graphic, where no slicing can happen because its a pimpl-class.
Yes, I know OpenGrok brings up the UNO API one first :-) Look where the decompose of GraphicPrimitive2D produces a GraphicPrimitive2D, this cannot happen AFAIK the code. Maybe the same object stays (it"s a uno reference) for some compiler error or so...?
Okay, look for why the call process(rCandidate.get2DDecomposition(getViewInformation2D())); in AnimatedExtractingProcessor2D returns again a GraphicPrimitive2D or a sequence containing one. This should be impossible, see GraphicPrimitive2D::get2DDecomposition itself. here must be something different on mac 64bit (32bit works AFAIK)
Having debugged more into it the problematic primitive is a MaskPrimitive2D. It is derived from GroupPrimitive2D. When the recursion shows, then the MaskPrimitive2D is decomposed into the children of its GroupPrimitive2D with the only child being the original MaskPrimitive2D itself. The suspect place where the first child is set to the parent is in graphicprimitivehelper2d.cxx:762 aRetval[0] = new MaskPrimitive2D( basegfx::B2DPolyPolygon(aMaskPolygon), aRetval); with aRetVal being the sequence of children. Since the sequence only manages References assigning the object to be its own first child updates the objects children list too, so the endless recursion starts.
Having debugged into this on Linux/Windows/Mac64 the difference in the problematic code I pointed out above (graphicprimitivehelper2d.cxx:762) is that some compilers - allocate the new MaskPrimitive, then construct it, then assign the pointer while other compilers (such as on Xcode's clang) - allocate the new MaskPrimitive, assign its pointer, then construct it Its a similar situation with the ordering of subexpressions such as i = 2 * (i++); where you CANNOT rely on the increment being done before the assignment (C++ standard 5.0.4) or with argument evaluation order (C++ standard 5.2.2.8), i.e. for a call fn1( fn2(), fn3()); you CANNOT rely on fn2 being called before fn3. If a construct has an undefined order according to the standard, but a result depends on a specific order, then this construct must be sanitized. Automatically detecting such problems introduced by such undefined-order constructs is AFAIK not yet possible, especially like the situation here where multiple different modules are involved. E.g. coverity didn't catch it.
okay, so: aRetval[0] = new MaskPrimitive2D( basegfx::B2DPolyPolygon(aMaskPolygon), aRetval); on mac: - alloc: mem for MaskPrimitive2D - assign: aRetval[0] gets set (didi I get thar right?) - construct: constructor of MaskPrimitive2D, using aRetval else: - alloc, construct, assign... This leaves questions: - aren't there a lot of methods in c++ which return a reference to the object (*this), so retval usage before constructor should not be standard, should it ?!? - Does this (at least) clarify that the Sequence< B2DPrimitive > (the aRetval) internally gets copied, not only refcounted, also on mac ? Please clarify.
Doing a grep over the whole code to identify evtl. other places where this may be problematic with primitives...
Only found that single case we already talkabout. @Herbert: Have you already checked if the alternative I showed works?
Got cppu with debug and analyzed deeper; the original statement does first alloc mem, then call constructor. In constructor the sequence gets refcounted locally, not copied. The access to it using operator[] *does* explicitely create a copy before assigment of the new reference (not the operator[] const), so all okay in win version. If mac really does asign first and then call constructor, the problem is clear. Changing statement to hold a temp ptr to the result. BTW: The MaskPrimitive2D is only added since the metafile imported has a bigger internal size than the PrefSize says. Unfortunately it has shown over time that such metafiles exist, what should not be the case. Checked with changed statement, works the same on win and should avoid the critical section for compilers who do things differently. Please check using MaskPrimitive2D* pMaskPrimitive2D = new MaskPrimitive2D( basegfx::B2DPolyPolygon(aMaskPolygon), aRetval); aRetval[0] = pMaskPrimitive2D; instead of the previous version.
There is one other possibility except the alloc-assign-construct order; could you check with the original if the operator[] or operator[] const is used in the original by the mac version? It should be the non-const, but just to make sure...
Regarding the question in comment 24 whether the const or non-const operator is used: its the non-const version. But the ref-count is one at that time, so it isn't copied. The patch in comment 23 works fine too and looks clean, so I suggest to go with it.
Thanks for clarifying, I added the more compiler-independent version and a comment and will commit that.
Okay, done.
"alg" committed SVN revision 1569417 into trunk: i124073 choose a more compiler-independent way of construction
Verified in the the latest nightly build. Fixing the problematic code location that I pointed out solved it.