[Webkit-unassigned] [Bug 22620] Use ActiveDOMObject as base class for DOMTimers

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Dec 3 03:10:15 PST 2008


ap at webkit.org changed:

           What    |Removed                     |Added
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1

------- Comment #4 from ap at webkit.org  2008-12-03 03:10 PDT -------
This would be easier to review if ChangeLog explained the reasons for
JSDOMWindowBase changes, and why making equivalent calls in FrameLoader is
indeed equivalent.

+    ScriptExecutionContext* context = scriptExecutionContext();
+    if (context && context->isDocument()) {

The check for context being non-zero looks suspicious. Why is is possible for
the timer to fire after the context is destroyed? This is wrong for two
reasons: (1) DOMTimer::contextDestroyed() deletes the object, and (2) if we
needed to check for the context, we should have checked that we are not firing
the timer in a window that has replaced our window due to navigation (which
would be a security problem).

I don't know we have any tests for timers and navigation.

-            // An object with pending activity must have a wrapper to mark its
listeners, so no null check.
-            if (!wrapper->marked())
+            // Some ActiveDOMObjects don't have JS wrappers (timers created by
setTimeout is one example).

Maybe you could preserve part of the old comment, explaining that active
objects that can have wrappers must have them, because they are needed to mark
listeners. This is a rather subtle point, and the code may need to be revised
in the future (I don't believe in protected listeners' correctness).

+    TimeoutsMap timeouts;

I know you just copied the code, but this is a good opportunity to fix the
name, making it m_timeouts. It's sad that we have to track timeouts in Document
twice - once as ActiveDOMObject, and again in TimeoutMap - but fixing that is
probably out of scope for this patch.

+    if (Document* document = frame->document()) {
+        if (paused)
+            document->suspendActiveDOMObjects();
+        else
+            document->resumeActiveDOMObjects();

It's not quite obvious that objects that cannot be suspended will still get
suspend/resume calls sometimes. I think that this warrants a comment in both
ActiveDOMObject.h and ScriptExecutionContext.h.

+                if (Document* document = frame->document())
+                    document->suspendActiveDOMObjects();

I don't think we can have document-less frames now, even though there's still a
lot of code that makes this check.

Keeping the patch for review to give a chance to someone more familiar with
timer/loader interaction to weigh in, but this looks very good to me!

Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

More information about the webkit-unassigned mailing list