Bug 22620 - Use ActiveDOMObject as base class for DOMTimers
Summary: Use ActiveDOMObject as base class for DOMTimers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-12-03 00:23 PST by Dmitry Titov
Modified: 2008-12-04 00:42 PST (History)
1 user (show)

See Also:


Attachments
Proposed patch (25.11 KB, patch)
2008-12-03 00:34 PST, Dmitry Titov
no flags Details | Formatted Diff | Diff
patch v2 (24.65 KB, patch)
2008-12-03 01:22 PST, Dmitry Titov
no flags Details | Formatted Diff | Diff
patch v3 (24.27 KB, patch)
2008-12-03 01:34 PST, Dmitry Titov
no flags Details | Formatted Diff | Diff
patch v4 (post-review changes) (27.57 KB, patch)
2008-12-03 12:35 PST, Dmitry Titov
no flags Details | Formatted Diff | Diff
patch v5 (28.20 KB, patch)
2008-12-03 19:33 PST, Dmitry Titov
ap: review+
Details | Formatted Diff | Diff
patch to fix non-mac bulid (1021 bytes, patch)
2008-12-04 00:34 PST, Dmitry Titov
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Titov 2008-12-03 00:23:36 PST
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.
Comment 1 Dmitry Titov 2008-12-03 00:34:46 PST
Created attachment 25702 [details]
Proposed patch
Comment 2 Dmitry Titov 2008-12-03 01:22:53 PST
Created attachment 25705 [details]
patch v2
Comment 3 Dmitry Titov 2008-12-03 01:34:38 PST
Created attachment 25706 [details]
patch v3

Updated ChangeLog too, since ScriptExecutionContext is not modified anymore.
Comment 4 Alexey Proskuryakov 2008-12-03 03:10:15 PST
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!
Comment 5 Alexey Proskuryakov 2008-12-03 11:09:49 PST
See also bug 22630 (the existing calls to stopActiveDOMObjects are not sufficient).
Comment 6 Dmitry Titov 2008-12-03 12:24:47 PST
(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.
Comment 7 Dmitry Titov 2008-12-03 12:35:13 PST
Created attachment 25719 [details]
patch v4 (post-review changes)

Addressed comments by ap. Also sync'ed.
Comment 8 Dmitry Titov 2008-12-03 14:32:09 PST
From IRC:
ap: dimich: your updated patch looks good - but it will need to be updated for bug 22630's fix

working on it...
Comment 9 Dmitry Titov 2008-12-03 19:33:44 PST
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.
Comment 10 Alexey Proskuryakov 2008-12-03 23:05:54 PST
Comment on attachment 25733 [details]
patch v5

I feel sufficiently confident about this code now, r=me.
Comment 11 Alexey Proskuryakov 2008-12-03 23:19:38 PST
Committed revision 38985.

Comment 12 Dmitry Titov 2008-12-04 00:34:53 PST
Created attachment 25736 [details]
patch to fix non-mac bulid

non-mac builds are broken... need to add couple of brackets..
Comment 13 Alexey Proskuryakov 2008-12-04 00:38:21 PST
Comment on attachment 25736 [details]
patch to fix non-mac bulid

r=me
Comment 14 Alexey Proskuryakov 2008-12-04 00:42:56 PST
Build fix committed revision 38990.