Bug 22620 – Use ActiveDOMObject as base class for DOMTimers

• Component: DOM
• Status: RESOLVED FIXED

[Dmitry Titov]

This allows to consolidate separate calls like clearAllTimeouts/pauseTimeouts/resumeTimeouts into similar calls for ActiveDOMObjects. That removes timeouts-specific clear/pause/resume code.
It also makes it unnecessary to unload timers into PausedTimeout 'containers' on pause/resume - they can be paused/resumed in-place now, as ActiveDOMObjects. This makes it possible to get rid of PausedTimeouts as a class.
Finally, the hash table that keeps mapping id->timeout is moved from JSDOMWindow to Document to match the lifetime of DOMTimers.

This patch is a step (I'm trying to do one change at a time) towards making timeouts available on Workers.
Next steps (preliminary, some need more discussion): 
1. Delete file PausedTimeouts.h,cpp from the build
2. Since there is no JS* objects in DOMTimer anymore, it can be moved from bindings/js/ to lets say dom/
3. DOMTimer::fired() and JSDOMWindowBase::timerFired can be merged into one method on DOMTimer
4. Create/removeTimeout methods can be moved from JSDOMWindowBase to ScriptExecutionContext so they are available of Workers too. Move id->timer map to SEC at the same time.
5. Make DOMTimer able to post/receive time events on threads different from main UI one.

[Dmitry Titov]

Created attachment 25702 [details]
Proposed patch

[Dmitry Titov]

Created attachment 25705 [details]
patch v2

[Dmitry Titov]

Created attachment 25706 [details]
patch v3

Updated ChangeLog too, since ScriptExecutionContext is not modified anymore.

[Alexey Proskuryakov]

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!

[Alexey Proskuryakov]

See also bug 22630 (the existing calls to stopActiveDOMObjects are not sufficient).

[Dmitry Titov]

(In reply to comment #4)
Thanks for review! Upcoming patch addresses all the comments.

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

I'll add one soon if we don't - made a note for myself :-)

> +                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.

I thought of replacing with ASSERT but there are so many places around that do exactly the same that it feels a blanket change could be done, in a separate patch.

[Dmitry Titov]

Created attachment 25719 [details]
patch v4 (post-review changes)

Addressed comments by ap. Also sync'ed.

[Dmitry Titov]

From IRC:
ap: dimich: your updated patch looks good - but it will need to be updated for bug 22630's fix

working on it...

[Dmitry Titov]

Created attachment 25733 [details]
patch v5

Reflected the changes for bug 22630, created 2 tests (to be sent as separate patch after a bit of cleanup), debugged through CachedPage suspense/resume and debugger step. Browsed with debug bits. Seems good :-)

Hope someone familiar with timers/loading could take a look.

[Alexey Proskuryakov]

Comment on attachment 25733 [details]
patch v5

I feel sufficiently confident about this code now, r=me.

[Alexey Proskuryakov]

Committed revision 38985.

[Dmitry Titov]

Created attachment 25736 [details]
patch to fix non-mac bulid

non-mac builds are broken... need to add couple of brackets..

[Alexey Proskuryakov]

Comment on attachment 25736 [details]
patch to fix non-mac bulid

r=me

[Alexey Proskuryakov]

Build fix committed revision 38990.