[Gnash-commit] /srv/bzr/gnash/trunk r12227: Do not construct MovieClips that have no onUnload handler if they are placed

[Benjamin Wolsey]

------------------------------------------------------------
revno: 12227 [merge]
committer: Benjamin Wolsey <address@hidden>
branch nick: trunk
timestamp: Sun 2010-06-06 14:11:46 +0200
message:
  Do not construct MovieClips that have no onUnload handler if they are placed
  and removed in skipped frames.
  
  This involves two changes: a) allow all ExecutableCode to be identified
  by its target, and b) allow removal of queued constructors by target.
  
  Tests in misc-ming.all/RegisterClass3Test pass.
  
  Fixes bug #29771.
modified:
  libcore/DisplayObject.cpp
  libcore/MovieClip.cpp
  libcore/MovieClip.h
  libcore/MovieLoader.cpp
  libcore/movie_root.cpp
  libcore/movie_root.h
  libcore/swf/RemoveObjectTag.cpp
  libcore/vm/ExecutableCode.h
  testsuite/actionscript.all/TextFormat.as
  testsuite/misc-ming.all/RegisterClassTest3.c

=== modified file 'libcore/DisplayObject.cpp'
--- a/libcore/DisplayObject.cpp 2010-06-05 09:00:48 +0000
+++ b/libcore/DisplayObject.cpp 2010-06-06 11:27:45 +0000
@@ -481,6 +481,8 @@
 
        const bool hasEvent = hasEventHandler(event_id::UNLOAD);
 
+    if (!hasEvent) stage().removeQueuedConstructor(this);
+
        _unloaded = true;
 
        return hasEvent;

=== modified file 'libcore/MovieClip.cpp'
--- a/libcore/MovieClip.cpp     2010-06-03 17:32:50 +0000
+++ b/libcore/MovieClip.cpp     2010-06-06 10:17:39 +0000
@@ -101,7 +101,7 @@
 
     explicit ConstructEvent(MovieClip* nTarget)
         :
-        _target(nTarget)
+        ExecutableCode(nTarget)
     {}
 
 
@@ -112,24 +112,8 @@
 
     virtual void execute()
     {
-        _target->constructAsScriptObject();
-    }
-
-#ifdef GNASH_USE_GC
-    /// Mark reachable resources (for the GC)
-    //
-    /// Reachable resources are:
-    ///    - the action target (_target)
-    ///
-    virtual void markReachableResources() const
-    {
-        _target->setReachable();
-    }
-#endif // GNASH_USE_GC
-
-private:
-
-    MovieClip* _target;
+        static_cast<MovieClip*>(target())->constructAsScriptObject();
+    }
 
 };
 
@@ -1290,6 +1274,13 @@
 }
 
 void
+MovieClip::remove_display_object(int depth, int)
+{
+    set_invalidated();
+    _displayList.removeDisplayObject(depth);
+}
+
+void
 MovieClip::increment_frame_and_check_for_loop()
 {
     const size_t frame_count = get_loaded_frames(); 

=== modified file 'libcore/MovieClip.h'
--- a/libcore/MovieClip.h       2010-06-03 17:32:50 +0000
+++ b/libcore/MovieClip.h       2010-06-06 10:17:39 +0000
@@ -351,11 +351,7 @@
     /// (2)the id might be used for specifying a DisplayObject
     /// in the depth(think about multiple DisplayObjects within the same
     /// depth, not tested and a rare case)
-    void remove_display_object(int depth, int /* id */)
-    {
-        set_invalidated();
-        _displayList.removeDisplayObject(depth);
-    }
+    void remove_display_object(int depth, int /*id*/);
 
     void unloadMovie();
 

=== modified file 'libcore/MovieLoader.cpp'
--- a/libcore/MovieLoader.cpp   2010-06-03 17:32:50 +0000
+++ b/libcore/MovieLoader.cpp   2010-06-06 10:17:39 +0000
@@ -324,7 +324,8 @@
         // onLoadInit call next, so it happens after the former.
         //
         std::auto_ptr<ExecutableCode> code(
-                new DelayedFunctionCall(handler, NSV::PROP_BROADCAST_MESSAGE, 
+                new DelayedFunctionCall(targetDO, handler,
+                    NSV::PROP_BROADCAST_MESSAGE, 
                     "onLoadInit", getObject(targetDO)));
 
         getRoot(*handler).pushAction(code, movie_root::PRIORITY_DOACTION);

=== modified file 'libcore/movie_root.cpp'
--- a/libcore/movie_root.cpp    2010-06-06 07:33:08 +0000
+++ b/libcore/movie_root.cpp    2010-06-06 11:27:45 +0000
@@ -86,27 +86,28 @@
 
 namespace gnash {
 
+// Forward declarations
 namespace {
     bool generate_mouse_button_events(movie_root& mr, MouseButtonState& ms);
     const DisplayObject* getNearestObject(const DisplayObject* o);
     as_object* getBuiltinObject(movie_root& mr, string_table::key cl);
 }
 
-}
-
-namespace gnash {
-
-inline bool
-movie_root::testInvariant() const
+// Utility classes
+namespace {
+
+class FindTarget
 {
-    // TODO: fill this function !
-    // The _movies map can not invariantably
-    // be non-empty as the stage is autonomous
-    // itself
-    //assert( ! _movies.empty() );
+public:
+    FindTarget(DisplayObject* target) : _target(target) {}
+    bool operator()(ExecutableCode* c) const {
+        return _target == c->target();
+    }
+private:
+    DisplayObject* _target;
+};
 
-    return true;
-}
+} // anonymous namespace
 
 
 movie_root::movie_root(const movie_definition& def,
@@ -1540,6 +1541,23 @@
 }
 
 void
+movie_root::removeQueuedConstructor(DisplayObject* target)
+{
+
+    ActionQueue& pr = _actionQueue[PRIORITY_CONSTRUCT];
+    
+    const size_t st = pr.size();
+
+    pr.erase(std::remove_if(pr.begin(), pr.end(), FindTarget(target)),
+            pr.end());
+
+    const size_t sa = pr.size();
+
+    log_debug("%s items removed from ActionQueue (size now: %s)",
+            st - sa, sa);
+}
+
+void
 movie_root::pushAction(std::auto_ptr<ExecutableCode> code, size_t lvl)
 {
     assert(lvl < PRIORITY_SIZE);
@@ -2398,6 +2416,18 @@
     return true;
 }
 
+inline bool
+movie_root::testInvariant() const
+{
+    // TODO: fill this function !
+    // The _movies map can not invariantably
+    // be non-empty as the stage is autonomous
+    // itself
+    //assert( ! _movies.empty() );
+
+    return true;
+}
+
 namespace {
 
 // Return whether any action triggered by this event requires display redraw.

=== modified file 'libcore/movie_root.h'
--- a/libcore/movie_root.h      2010-06-02 14:18:20 +0000
+++ b/libcore/movie_root.h      2010-06-06 11:27:45 +0000
@@ -930,6 +930,14 @@
     std::string callExternalCallback(const std::string &name, 
                                      const std::vector<as_value>& args);
 
+    /// Removes a queued constructor from the execution queue
+    //
+    /// This is used to prevent construction of targets that are placed and
+    /// then removed in skipped frames. Callers are responsible for determining
+    /// whether it should be removed, for instance by checking for an 
+    /// onUnload handler.
+    void removeQueuedConstructor(DisplayObject* target);
+
 private:
 
     /// Set the root movie, replacing the current one if any.

=== modified file 'libcore/swf/RemoveObjectTag.cpp'
--- a/libcore/swf/RemoveObjectTag.cpp   2010-05-20 09:57:10 +0000
+++ b/libcore/swf/RemoveObjectTag.cpp   2010-06-02 11:04:47 +0000
@@ -51,7 +51,7 @@
 RemoveObjectTag::executeState(MovieClip* m, DisplayList& dlist) const
 {
     m->set_invalidated();
-       dlist.removeDisplayObject(_depth);
+    dlist.removeDisplayObject(_depth);
 }
 
 /* public static */

=== modified file 'libcore/vm/ExecutableCode.h'
--- a/libcore/vm/ExecutableCode.h       2010-05-07 08:21:15 +0000
+++ b/libcore/vm/ExecutableCode.h       2010-06-02 09:27:08 +0000
@@ -33,7 +33,8 @@
 class ExecutableCode {
 
 public:
-    ExecutableCode() {}
+
+    ExecutableCode(DisplayObject* t) : _target(t) {}
 
     virtual void execute()=0;
 
@@ -41,10 +42,23 @@
 
     virtual ~ExecutableCode() {}
 
+    virtual void setReachable() const {}
+
 #ifdef GNASH_USE_GC
     /// Mark reachable resources (for the GC)
-    virtual void markReachableResources() const=0;
-#endif // GNASU_USE_GC
+    void markReachableResources() const {
+        setReachable();
+        if (_target) _target->setReachable();
+    }
+#endif 
+
+    DisplayObject* target() const { 
+        return _target;
+    }
+
+private:
+
+    DisplayObject* _target;
 };
 
 /// Global code (out of any function)
@@ -54,8 +68,8 @@
 
     GlobalCode(const action_buffer& nBuffer, DisplayObject* nTarget)
         :
-        buffer(nBuffer),
-        target(nTarget)
+        ExecutableCode(nTarget),
+        buffer(nBuffer)
     {}
 
     ExecutableCode* clone() const
@@ -65,28 +79,16 @@
 
     virtual void execute()
     {
-        if (!target->unloaded()) {
-            ActionExec exec(buffer, target->get_environment());
+        if (!target()->unloaded()) {
+            ActionExec exec(buffer, target()->get_environment());
             exec();
         }
     }
 
-#ifdef GNASH_USE_GC
-    /// Mark reachable resources (for the GC)
-    //
-    /// Reachable resources are:
-    ///  - the action target (target)
-    ///
-    virtual void markReachableResources() const {
-        if (target) target->setReachable();
-    }
-#endif // GNASU_USE_GC
-
 private:
 
     const action_buffer& buffer;
 
-    DisplayObject* target;
 };
 
 /// Event code 
@@ -98,12 +100,12 @@
 
     EventCode(DisplayObject* nTarget)
         :
-        _target(nTarget)
+        ExecutableCode(nTarget)
     {}
 
     EventCode(DisplayObject* nTarget, const BufferList& buffers)
         :
-        _target(nTarget),
+        ExecutableCode(nTarget),
         _buffers(buffers)
     {}
 
@@ -124,7 +126,7 @@
     {
         // don't push actions for destroyed DisplayObjects, 
         // our opcode guard is bogus at the moment.
-        if (!_target->isDestroyed()) {
+        if (!target()->isDestroyed()) {
             _buffers.push_back(&buffer);
         }
     }
@@ -136,29 +138,15 @@
         {
             // onClipEvents code are guarded by isDestroyed(),
             // still might be also guarded by unloaded()
-            if (_target->isDestroyed())  break;
+            if (target()->isDestroyed())  break;
 
-            ActionExec exec(*(*it), _target->get_environment(), false);
+            ActionExec exec(*(*it), target()->get_environment(), false);
             exec();
         }
     }
 
-#ifdef GNASH_USE_GC
-    /// Mark reachable resources (for the GC)
-    //
-    /// Reachable resources are:
-    ///  - the action target (_target)
-    ///
-    virtual void markReachableResources() const
-    {
-        if ( _target ) _target->setReachable();
-    }
-#endif // GNASU_USE_GC
-
 private:
 
-    DisplayObject* _target;
-
     BufferList _buffers;
 
 };
@@ -170,7 +158,7 @@
 
     QueuedEvent(DisplayObject* nTarget, const event_id& id)
         :
-        _target(nTarget),
+        ExecutableCode(nTarget),
         _eventId(id)
     {}
 
@@ -183,28 +171,14 @@
     virtual void execute()
     {
         // don't execute any events for destroyed DisplayObject.
-        if( !_target->isDestroyed() )
+        if (!target()->isDestroyed() )
         {
-            _target->notifyEvent(_eventId);
+            target()->notifyEvent(_eventId);
         }
     }
 
-#ifdef GNASH_USE_GC
-    /// Mark reachable resources (for the GC)
-    //
-    /// Reachable resources are:
-    ///  - the action target (_target)
-    ///
-    virtual void markReachableResources() const
-    {
-        if ( _target ) _target->setReachable();
-    }
-#endif // GNASU_USE_GC
-
 private:
 
-    DisplayObject* _target;
-
     const event_id _eventId;
 
 };
@@ -225,10 +199,12 @@
 
 public:
 
-    DelayedFunctionCall(as_object* target, string_table::key name,
+    DelayedFunctionCall(DisplayObject* target,
+            as_object* obj, string_table::key name,
             const as_value& arg1, const as_value& arg2)
         :
-        _target(target),
+        ExecutableCode(target),
+        _obj(obj),
         _name(name),
         _arg1(arg1),
         _arg2(arg2)
@@ -242,7 +218,7 @@
 
     virtual void execute()
     {
-        callMethod(_target, _name, _arg1, _arg2);
+        callMethod(_obj, _name, _arg1, _arg2);
     }
 
 #ifdef GNASH_USE_GC
@@ -251,17 +227,17 @@
     /// Reachable resources are:
     ///  - the action target (_target)
     ///
-    virtual void markReachableResources() const
+    virtual void setReachable() const
     {
-      _target->setReachable();
-      _arg1.setReachable();
-      _arg2.setReachable();
+        _obj->setReachable();
+        _arg1.setReachable();
+        _arg2.setReachable();
     }
 #endif // GNASH_USE_GC
 
 private:
 
-    as_object* _target;
+    as_object* _obj;
     string_table::key _name;
     as_value _arg1, _arg2;
 

=== modified file 'testsuite/actionscript.all/TextFormat.as'
--- a/testsuite/actionscript.all/TextFormat.as  2010-05-25 11:26:05 +0000
+++ b/testsuite/actionscript.all/TextFormat.as  2010-06-03 08:24:51 +0000
@@ -381,6 +381,11 @@
 te = tf2.getTextExtent("Longer sentence with more words.", 30);
 check_equals(te.textFieldWidth, 30);
 xcheck_equals(Math.round(te.width), 25);
+#if OUTPUT_VERSION > 7
+xcheck_equals(te.height, 152.9);
+#else
+xcheck_equals(te.height, 152);
+#endif
 
 te = tf2.getTextExtent("o");
 xcheck_equals(Math.round(te.textFieldWidth), 12);
@@ -432,7 +437,7 @@
 #if OUTPUT_VERSION < 7
     check_totals(122);
 #elif OUTPUT_VERSION == 7
-    check_totals(158);
+    check_totals(159);
 #else 
-    check_totals(158);
+    check_totals(159);
 #endif

=== modified file 'testsuite/misc-ming.all/RegisterClassTest3.c'
--- a/testsuite/misc-ming.all/RegisterClassTest3.c      2010-05-10 07:58:53 
+0000
+++ b/testsuite/misc-ming.all/RegisterClassTest3.c      2010-06-06 11:04:13 
+0000
@@ -13,7 +13,7 @@
 {
     SWFMovie mo;
     SWFMovieClip mc3, mc2, dejagnuclip;
-    SWFAction ac, ac1;
+    SWFAction ac, ac1, initac;
     SWFDisplayItem it;
     SWFShape sha;
 
@@ -29,6 +29,7 @@
     SWFMovie_setDimension(mo, 800, 600);
     SWFMovie_setRate (mo, 12.0);
 
+
     sha = newSWFShape();
 
     // Character ID 2. Has 1 showframe. Is exported first.
@@ -41,7 +42,8 @@
     SWFMovie_writeExports(mo);
 
     // Main timeline actions for frame 1
-    add_actions(mo, "var c = 0; trace('frame 1'); gotoAndStop(3);");
+    add_actions(mo, "var c = 0; var i = 0; trace('frame 1'); gotoAndStop(3);");
+    initac = newSWFAction("trace('onInitialize'); _root.i++;");
     
     // ID 3 is defined here. It has no showframe. It is exported immediately.
     mc3 = newSWFMovieClip();
@@ -52,11 +54,12 @@
                    0, 0, 800, 600);
     SWFMovie_add(mo, (SWFBlock)dejagnuclip);
 
-    // Init actions for ID 3
+    // Init actions for ID 3. This only exists to create the constructor
+    // for mc2 in its initactions.
     ac = newSWFAction(
     "   _global.ctor = function () {"
     "       super();"
-    "       trace('Object in Frame 2 is constructed');"
+    "       trace('Object ID 2 is constructed');"
     "       c += 1;"
     "   };"
     );
@@ -77,6 +80,7 @@
     // Place object ID 2.
     it = SWFMovie_add(mo, (SWFBlock)mc2);
     SWFDisplayItem_setName(it, "mc2");
+    SWFDisplayItem_addAction(it, initac, SWFACTION_INIT);
 
     // Frame 3
     SWFMovie_nextFrame(mo);
@@ -87,10 +91,12 @@
     add_actions(mo, "trace('frame 3');");
     
     // The class should not be constructed if the object is removed after
-    // being placed. It should be constructed if it's not removed.
-    xcheck(mo, "c == 0");
+    // being placed. It should be constructed if it's not removed. MovieClips
+    // with an onUnload handler are always constructed.
+    check_equals(mo, "c", "0");
+    check_equals(mo, "i", "0");
     check(mo, "_root.mc2 == undefined");
-    add_actions(mo, "gotoAndStop(5);");
+    add_actions(mo, "gotoAndPlay(5);");
 
     // Frame 4
     SWFMovie_nextFrame(mo);
@@ -99,6 +105,7 @@
     // Place object ID 2 again
     it = SWFMovie_add(mo, (SWFBlock)mc2);
     SWFDisplayItem_setName(it, "mc2a");
+    SWFDisplayItem_addAction(it, initac, SWFACTION_INIT);
 
     // Frame 5
     SWFMovie_nextFrame(mo);
@@ -107,8 +114,10 @@
     // so it should be present and the constructor should be
     // called.
     add_actions(mo, "trace('frame 5');");
-    xcheck(mo, "c == 1");
+    check_equals(mo, "c", "1");
+    check_equals(mo, "i", "1");
     check(mo, "typeof(_root.mc2a) == 'movieclip'");
+    add_actions(mo, "stop();");
 
     SWFMovie_nextFrame(mo);