Issue 124073 - endless recursion between AnimatedExtractingProcessor2D and BaseProcessor2D
Summary: endless recursion between AnimatedExtractingProcessor2D and BaseProcessor2D
Status: CLOSED FIXED
Alias: None
Product: Impress
Classification: Application
Component: code (show other issues)
Version: 4.1.0-dev
Hardware: All Mac OS X, all
: P3 Normal (vote)
Target Milestone: 4.1.0
Assignee: AOO issues mailing list
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-21 17:23 UTC by hdu@apache.org
Modified: 2017-05-20 10:35 UTC (History)
2 users (show)

See Also:
Issue Type: DEFECT
Latest Confirmation in: ---
Developer Difficulty: ---


Attachments
stack of the crash (3.03 KB, text/plain)
2014-01-22 10:27 UTC, hdu@apache.org
no flags Details
stack similar to the reported crash (3.08 KB, text/plain)
2014-01-22 10:30 UTC, hdu@apache.org
no flags Details
reduced bugdoc (76.50 KB, application/vnd.ms-powerpoint)
2014-01-22 16:01 UTC, hdu@apache.org
no flags Details

Note You need to log in before you can comment on or make changes to this issue.
Description hdu@apache.org 2014-01-21 17:23:25 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
Comment 1 hdu@apache.org 2014-01-21 17:29:18 UTC
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
Comment 2 Armin Le Grand 2014-01-21 17:35:50 UTC
Taking a look...
Comment 3 Armin Le Grand 2014-01-21 17:57:43 UTC
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.
Comment 4 Armin Le Grand 2014-01-21 18:21:20 UTC
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?
Comment 5 hdu@apache.org 2014-01-22 10:26:40 UTC
The crash could also be in slide 22 (i.e. the last slide one sees is slide 21).
Comment 6 hdu@apache.org 2014-01-22 10:27:49 UTC
Created attachment 82351 [details]
stack of the crash

always alternating between viewobjectcontact.cxx:152 and baseprocessor2d.cxx:71 until crash
Comment 7 hdu@apache.org 2014-01-22 10:30:12 UTC
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 8 hdu@apache.org 2014-01-22 10:31:37 UTC
Comment on attachment 82352 [details]
stack similar to the reported crash

always alternating between vclmetafileprocessor2d.cxx:2140 and baseprocessor2d.cxx:71 instead
Comment 9 Armin Le Grand 2014-01-22 14:48:45 UTC
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!
Comment 10 Armin Le Grand 2014-01-22 14:58:59 UTC
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)?
Comment 11 hdu@apache.org 2014-01-22 16:01:07 UTC
Created attachment 82364 [details]
reduced bugdoc

With the reduced bugdoc the crash now happens immediately.
Comment 12 Armin Le Grand 2014-01-22 20:39:51 UTC
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!
Comment 13 Armin Le Grand 2014-01-23 13:29:12 UTC
@Herbert: have you seen #124005#, may also be Mac64bit specific with VCL controls ?!? Just a thought...
Comment 14 hdu@apache.org 2014-01-23 13:47:44 UTC
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?
Comment 15 hdu@apache.org 2014-01-23 14:32:27 UTC
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.
Comment 16 Armin Le Grand 2014-01-23 17:51:58 UTC
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...?
Comment 17 Armin Le Grand 2014-01-24 16:08:49 UTC
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)
Comment 18 hdu@apache.org 2014-02-12 15:25:09 UTC
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.
Comment 19 hdu@apache.org 2014-02-13 10:05:15 UTC
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.
Comment 20 Armin Le Grand 2014-02-13 13:24:19 UTC
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.
Comment 21 Armin Le Grand 2014-02-17 17:14:03 UTC
Doing a grep over the whole code to identify evtl. other places where this may be problematic with primitives...
Comment 22 Armin Le Grand 2014-02-17 17:31:35 UTC
Only found that single case we already talkabout.
@Herbert: Have you already checked if the alternative I showed works?
Comment 23 Armin Le Grand 2014-02-17 17:56:44 UTC
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.
Comment 24 Armin Le Grand 2014-02-17 19:25:07 UTC
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...
Comment 25 hdu@apache.org 2014-02-18 09:24:21 UTC
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.
Comment 26 Armin Le Grand 2014-02-18 16:37:51 UTC
Thanks for clarifying, I added the more compiler-independent version and a comment and will commit that.
Comment 27 Armin Le Grand 2014-02-18 16:38:19 UTC
Okay, done.
Comment 28 SVN Robot 2014-02-18 16:43:50 UTC
"alg" committed SVN revision 1569417 into trunk:
i124073 choose a more compiler-independent way of construction
Comment 29 hdu@apache.org 2014-02-20 07:58:45 UTC
Verified in the the latest nightly build. Fixing the problematic code location that I pointed out solved it.