Fri 01 Nov 2013 09:11:38 PM UTC, comment #14:
What if we just make the destructor do nothing, rather than trying to clear children ?
|
Thu 31 Oct 2013 09:18:00 PM UTC, comment #13:
Another related issue is bug #40440
all the worm off the can...
|
Thu 31 Oct 2013 08:47:08 PM UTC, comment #12:
See bug #40439 for a possible regression introduced by the fix for this. Luckly we have a testcase for this bug, so it'll be easier to find a solution for the new bug (which will need a testcase on itself)
|
Thu 04 Jul 2013 02:18:02 PM UTC, comment #11:
Thanks!
|
Thu 04 Jul 2013 01:23:26 PM UTC, comment #10:
Tested with
http://git.savannah.gnu.org/gitweb/?p=gnash.git;a=commitdiff;h=fb23f4d
|
Thu 04 Jul 2013 01:01:14 PM UTC, comment #9:
Here's a test that makes valgrind capable of finding the problem:
var x = new XML('<t></t>');
x.appendChild(new XML('<t></t>'));
var x2 = new XML('<t></t>');
x2.appendChild(x);
// many allocations force GC run
for (var i=0; i<10000; ++i) x = {};
Build in version 6, possibly uncompressed.
Run with gprocessor. Valgrind reports:
==9087== Invalid read of size 8
==9087== at 0x51EDA1C: gnash::XMLNode_as::clearChildren() (XMLNode_as.cpp:384)
==9087== by 0x51EDA95: gnash::XMLNode_as::~XMLNode_as() (XMLNode_as.cpp:113)
==9087== by 0x51EBFB0: gnash::XML_as::~XML_as() (XML_as.h:77)
==9087== by 0x50A6491: gnash::as_object::~as_object() (checked_delete.hpp:34)
==9087== by 0x564DF9E: gnash::GC::~GC() (GC.cpp:62)
==9087== by 0x50E5B41: gnash::movie_root::~movie_root() (movie_root.cpp:190)
==9087== by 0x111CCE: play_movie(std::string const&, gnash::RunResources const&) (processor.cpp:602)
==9087== by 0x110AFB: main (processor.cpp:368)
==9087== Address 0x15f8c620 is 32 bytes inside a block of size 136 free'd
==9087== at 0x4C2A4BC: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==9087== by 0x50A6491: gnash::as_object::~as_object() (checked_delete.hpp:34)
==9087== by 0x564DF9E: gnash::GC::~GC() (GC.cpp:62)
==9087== by 0x50E5B41: gnash::movie_root::~movie_root() (movie_root.cpp:190)
==9087== by 0x111CCE: play_movie(std::string const&, gnash::RunResources const&) (processor.cpp:602)
==9087== by 0x110AFB: main (processor.cpp:368)
|
Thu 04 Jul 2013 11:19:30 AM UTC, comment #8:
Yes, as explained on IRC if the GC cleans the parent before the child, the child will find it's _parent member to be NULL, as set by the parent's destructor (which won't access the child if the child was deleted before as it will unlink self from parent).
A testcase would need to run for multiple frames as GC only runs on frame boundary and only if enough GCResources were allocated since last run. So it'd take some onEnterFrame and big builds of XML, then deletion and more builds. Could be a fun task.
Maybe gg0 wants to try this one :)
|
Wed 03 Jul 2013 06:56:01 PM UTC, comment #7:
I think Sandro now understands the patch.
As for a testcase, so far I have unfortunately not figured how to reduce a problematic SWF into a testcase. The simplest movie I've found so far that exhibits the problem is this one:
http://www.myclubdesign.com/WebRoot/ce_fr3/Shops/270286/MediaGallery/animations_flash/logopartenaires.swf
|
Wed 03 Jul 2013 03:24:07 PM UTC, comment #6:
Bastiaan please drop a reference to a commit when closing tickets.
It looks like your change just shakes the perspective, not really fixing the problem:
http://git.savannah.gnu.org/gitweb/?p=gnash.git;a=commitdiff;h=c5f7578
The problem as I see it is that objects are assuming other objects they don't own are alive. What if the GC cleans the parent before the child ? We'd be back to memory error.
Could you add a simplified testcase to exploit the problem ?
|
Wed 03 Jul 2013 01:43:36 PM UTC, comment #5:
GcResources are owned by the GC, so if your theory is correct the bug is in the node checking its children (ie: assuming they are alive) at destruction time.
Could all child be GcResources ?
Or could being or not a GcResource be registered somewhere else ?
|
Wed 03 Jul 2013 12:55:08 PM UTC, comment #4:
So here's my theory.
Supposing a simple parsed XML tree:
XMLobj -> child -> grandchild
Where XMLobj is the root node, which is kept alive by AS. Let's assume XMLobj clears its tree by parsing a new XML document. Because XMLobj owns child, it can simply forget about child, and child will automatically be garbage-collected because child's setReachable will no longer be called. In turn, grandchild will also be garbage collected, because it is owned by child.
The complication arises when a node clears its children: because not all children are GcResources, it checks whether a child is a GcResource by testing if it has an associated as_object, and deletes if the child doesn't.
This works fine, but this presumes that child and grandchild are garbage-collected in order of precedence (i.e., parent first). As it turns out, the garbage collector is just as, or perhaps more, likely to destroy grandchild before child. And in that case, when child is deleted after grandchild, child attempts to probe grandchild, which is already deleted.
|
Wed 03 Jul 2013 10:01:30 AM UTC, comment #3:
Something is in reach of the XMLNode and not marking it...
What ? Hard to tell. Time to find the simplest case and simplify it further till you get to figure out.
|
Wed 03 Jul 2013 09:59:17 AM UTC, comment #2:
There is code serving that purpose in XMLNode::setReachable(), and it is evidently not being called, but I'm not sure why.
|
Wed 03 Jul 2013 09:56:39 AM UTC, comment #1:
XMLNode not marking childs as reachable ?
|
Wed 03 Jul 2013 09:39:29 AM UTC, original submission:
Valgrind shows the following error for quite a few movies, Youtube among which.
|