Bug 53202

Created attachment 80263 [details] Patch

Comment on attachment 80263 [details] Patch Rejecting attachment 80263 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'bu..." exit_code: 2 Last 500 characters of output: ntation . fast/dom/Range .............................. fast/dom/Selection .. fast/dom/SelectorAPI ................. fast/dom/StyleSheet ................ fast/dom/Text . fast/dom/TreeWalker ........ fast/dom/Window .................................... fast/dom/Window/timer-resume-on-navigation-back.html -> failed Exiting early after 1 failures. 7521 tests run. 194.73s total testing time 7520 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 4 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/7574361

It looks like this is preventing the document from going into the page cache because the default implementation of canSuspend returns false, and logCanCacheFrameDecision in PageCache.cpp checks for that. James, you may want to watch out for that in the patch on bug 53188. +Jeremy, since he expressed interest in using EventQueue. Jeremy, this patch makes the EventQueue constructor take a Document. Should it take a ScriptExecutionContext instead, if you want to use it in a worker?

+ ap because this is touching activeDOMObject (In reply to comment #3) > It looks like this is preventing the document from going into the page cache because the default implementation of canSuspend returns false, and logCanCacheFrameDecision in PageCache.cpp checks for that. > > James, you may want to watch out for that in the patch on bug 53188. > > +Jeremy, since he expressed interest in using EventQueue. Jeremy, this patch makes the EventQueue constructor take a Document. Should it take a ScriptExecutionContext instead, if you want to use it in a worker? I don't need it in a worker yet, so it's probably fine to leave it as is for now. (I'm just asserting isDocument and then casting for now.) Mihai, do you want it to cause things not to suspend? If not, you should just override the canSuspend function.

(In reply to comment #4) > Mihai, do you want it to cause things not to suspend? If not, you should just override the canSuspend function. Yeah, I'm just going to override canSuspend.

Created attachment 80420 [details] Patch for landing

Comment on attachment 80420 [details] Patch for landing Wait, how can EventQueue be an ActiveDOMObject when it's not a DOM object, and doesn't have a JS wrapper?

(In reply to comment #7) > (From update of attachment 80420 [details]) > Wait, how can EventQueue be an ActiveDOMObject when it's not a DOM object, and doesn't have a JS wrapper? I think we need to pull the notification aspect of ActiveDOMObject out from it. It seems like this is a pretty common need, even though these things aren't technically "ActiveDOMObjects". (Which is why IndexedDB and VoidCallback use it.) Thoughts?

The notification aspect of ActiveDOMObject is poorly thought through. Stop/canSuspend/suspend don't really reflect what we need - "suspending" for debugger, for page cache and for modal dialogs are all different. Different kinds of modal dialogs (alert() vs. showModalDialog() vs. print()) aren't necessarily all the same either. See e.g. bug 45306 comment 9.

(In reply to comment #9) > The notification aspect of ActiveDOMObject is poorly thought through. Stop/canSuspend/suspend don't really reflect what we need - "suspending" for debugger, for page cache and for modal dialogs are all different. Different kinds of modal dialogs (alert() vs. showModalDialog() vs. print()) aren't necessarily all the same either. > > See e.g. bug 45306 comment 9. That was followed up by http://trac.webkit.org/changeset/67432, which added the ReasonForSuspension enum to ActiveDOMObject::suspend. Do you have in mind any further refactorings that would make this cleaner?

No, I don't have anything specific to suggest. If I were doing this, I'd have started with making a list of objects that need such notification, and their behaviors. We have a lot of objects that are notified in a custom way - see e.g. bug 24030 or bug 34679. There is one good thing about custom code though - AsyncDOMObject is not particularly scalable, as we have to iterate a list that includes even objects that don't care about the current reason for suspension. An object that notifies objects it owns can be more discriminate. AsyncDOMObject even iterates objects that are not active right now!

(In reply to comment #9) > The notification aspect of ActiveDOMObject is poorly thought through. Stop/canSuspend/suspend don't really reflect what we need - "suspending" for debugger, for page cache and for modal dialogs are all different. Different kinds of modal dialogs (alert() vs. showModalDialog() vs. print()) aren't necessarily all the same either. > > See e.g. bug 45306 comment 9. In this case we do want to treat all of those (debugger, page cage, modal dialogs) in the same way, so I'm not sure I see the issue. EventQueues are 1:1 with Documents so efficiency is not a problem either. Do you have a specific issue with using ActiveDOMObject other than the name and the fact that it has been used for a different use case previously?

How about this approach: 1. Create a Suspendable class, and move these methods from ActiveDOMObject into it: virtual bool canSuspend() const; virtual void suspend(ReasonForSuspension); virtual void resume(); virtual void stop(); 2. Make ActiveDOMObject inherit from Suspendable 3. Make the Suspendable constructor register instances with ScriptExecutionContext (which will put these in a separate list from m_activeDOMObjects) 4. Rename ScriptExecutionContext::suspend/resumeActiveDOMObjects to just ::suspend/resume, and have it loop over the list of Suspendables instead. 5. Make EventQueue (and RegisterAnimation) subclass Suspendable instead of ActiveDOMObject

Separating notification and GC protection responsibilities of ActiveDOMObjects is a reasonable step. I have some comments: - you probably don't want to have all suspendable objects in the list. E.g. if an animation isn't running, there is no reason to waste time on itin every suspend() call; - we do need to discuss specific needs of several interesting objects in detail to better assess the design - what exactly needs to be "suspended" in each case; - please don't make a special code path for EventQueue and RegisterAnimation only. Even if it's the best code path, it's not helpful if it adds to the mess of how objects are notified about document state changes. > 4. Rename ScriptExecutionContext::suspend/resumeActiveDOMObjects to just ::suspend/resume, > and have it loop over the list of Suspendables instead. This is not a great name - you won't suspend or resume ScriptExecutionContext itself in this method, all it does is notify some of the objects it tracks.

(In reply to comment #14) > Separating notification and GC protection responsibilities of ActiveDOMObjects is a reasonable step. I have some comments: > - you probably don't want to have all suspendable objects in the list. E.g. if an animation isn't running, there is no reason to waste time on it in every suspend() call; I can't speak for other use-cases, but there is one EventQueue and one RequestAnimationFrameController per Document, so the overhead of calling suspend on them is not significant. I'd rather not make the design even more complicated (by allowing Suspendables to add/remove themselves from the list at will) unless there's a specific need for it. > - we do need to discuss specific needs of several interesting objects in detail to better assess the design - what exactly needs to be "suspended" in each case; Does this need to be done now? By making ActiveDOMObject inherit from Suspendable, existing ActiveDOMObject subclasses will not be affected. I agree it makes sense to investigate other ActiveDOMObject implementations to see if they could just use Suspendable instead, but I think that's beyond the scope of one patch. > - please don't make a special code path for EventQueue and RegisterAnimation only. Even if it's the best code path, it's not helpful if it adds to the mess of how objects are notified about document state changes. What specifically do you think is special for EventQueue and RegisterAnimation? I'm just proposing splitting ActiveDOMObject into two. > This is not a great name - you won't suspend or resume ScriptExecutionContext itself in this method, all it does is notify some of the objects it tracks. ::suspendObjects perhaps? Any other suggestions?

Actually, now that I look deeper, I see that we already have WebCore/page/SuspendableTimer. I can just make EventQueue use that instead of Timer directly. However, that doesn't help with RequestAnimationFrameController. Alexey, would SuspendableTimer be a good candidate for subclassing my proposed Suspendable class instead of ActiveDOMObject?

Created attachment 80494 [details] Patch

The new patch avoids using ActiveDOMObject directly by using SuspendableTimer, which seems tailor-made for this purpose (the fact that SuspendableTimer uses ActiveDOMObject itself is debatable, but that should hopefully not block this patch).

Comment on attachment 80494 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80494&action=review > Source/WebCore/dom/Document.cpp:416 > + , m_eventQueue(adoptPtr(new EventQueue(this))) EventQueue should have a ::create() which returns a PassOwnPtr. new should pretty much never be outside of a ::create > Source/WebCore/dom/EventQueue.cpp:38 > +class EventQueueTimer : public SuspendableTimer { Is all of this just to avoid including SuspendableTimer.h in EventQueue.h? If so, I'm not sure it's worth it. > Source/WebCore/dom/EventQueue.cpp:41 > + explicit EventQueueTimer(EventQueue* eventQueue, ScriptExecutionContext* context) no explicit > Source/WebCore/dom/EventQueue.h:52 > + explicit EventQueue(ScriptExecutionContext*); Make private and add a factory method > Source/WebCore/dom/EventQueue.h:57 > + remove tab

Just a note: DRT now can test operations under modal dialogs using showModalDialog(). See http://trac.webkit.org/changeset/61599 for more details.

Created attachment 80504 [details] Patch

(In reply to comment #19) > EventQueue should have a ::create() which returns a PassOwnPtr. new should pretty much never be outside of a ::create Changed. > > Source/WebCore/dom/EventQueue.cpp:38 > > +class EventQueueTimer : public SuspendableTimer { > > Is all of this just to avoid including SuspendableTimer.h in EventQueue.h? If so, I'm not sure it's worth it. No, it's because SuspendableTimer (unlike Timer) does not take the method that you want called as a constructor argument, instead you have to implement fired(). > > Source/WebCore/dom/EventQueue.cpp:41 > > + explicit EventQueueTimer(EventQueue* eventQueue, ScriptExecutionContext* context) > > no explicit Oops, left over from when it only had one argument. Removed. > > Source/WebCore/dom/EventQueue.h:52 > > + explicit EventQueue(ScriptExecutionContext*); > > Make private and add a factory method Done. > > Source/WebCore/dom/EventQueue.h:57 > > + > > remove tab Done. (In reply to comment #20) > Just a note: DRT now can test operations under modal dialogs using showModalDialog(). See http://trac.webkit.org/changeset/61599 for more details. Thanks for the pointer, added a layout test.

(In reply to comment #22) > (In reply to comment #19) > > > Source/WebCore/dom/EventQueue.cpp:38 > > > +class EventQueueTimer : public SuspendableTimer { > > > > Is all of this just to avoid including SuspendableTimer.h in EventQueue.h? If so, I'm not sure it's worth it. > > No, it's because SuspendableTimer (unlike Timer) does not take the method that you want called as a constructor argument, instead you have to implement fired(). Sure, but why can't EventQueue implement fired rather than using its own method? I guess it's slightly less clear, but a comment would mostly fix that. I don't feel strongly though.

(In reply to comment #23) > Sure, but why can't EventQueue implement fired rather than using its own method? I guess it's slightly less clear, but a comment would mostly fix that. > > I don't feel strongly though. You mean have EventQueue subclass SuspendableTimer? That doesn't seem right.

Comment on attachment 80504 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80504&action=review Looks good to me. For posterity this behavior is specified by http://www.whatwg.org/specs/web-apps/current-work/multipage/timers.html#dom-alert (for alert(), confirm(), print()) and http://www.whatwg.org/specs/web-apps/current-work/multipage/timers.html#dom-showmodaldialog step 5 for showModalDialog(). > Source/WebCore/dom/EventQueue.h:59 > + virtual ~EventQueue(); why virtual? I can't find any subclasses of EventQueue in the codebase currently

Created attachment 80684 [details] Patch for landing

(In reply to comment #25) > > Source/WebCore/dom/EventQueue.h:59 > > + virtual ~EventQueue(); > > why virtual? I can't find any subclasses of EventQueue in the codebase currently No reason for it to be virtual, removed.

Comment on attachment 80684 [details] Patch for landing Clearing flags on attachment: 80684 Committed r77230: <http://trac.webkit.org/changeset/77230>

All reviewed patches have been landed. Closing bug.

Reopening since the change was rolled out in http://trac.webkit.org/changeset/77241 due to layout tests crashes on Chromium Debug bots: http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux%20(dbg)(1)/builds/787

Created attachment 80846 [details] Patch

Delta from previous patch is in SuspendableTimer, it was suspending the timer (and restarting it) even when it hadn't been started. I made SuspendableTimer keep track of whether or not the timer had been started via m_active.

Comment on attachment 80846 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80846&action=review Looks good, although I wonder if we shouldn't assimilate the m_suspend timer instead. Also while you're in SuspendableTimer.h can you make the c'tor explicit? > Source/WebCore/page/SuspendableTimer.h:50 > + bool m_active; nit: this should be declared after the doubles so it packs better.

Created attachment 80848 [details] Patch for landing

(In reply to comment #33) > Looks good, although I wonder if we shouldn't assimilate the m_suspend timer instead. As discussed in person, m_suspended is about making sure that suspend()/resume() calls are balanced (and is thus relevant to this being an ActiveDOMObject subclass), while m_active is about the timer being started, so they don't really do the same thing. > Also while you're in SuspendableTimer.h can you make the c'tor explicit? Done. > > Source/WebCore/page/SuspendableTimer.h:50 > > + bool m_active; > > nit: this should be declared after the doubles so it packs better. Done.

Comment on attachment 80848 [details] Patch for landing Clearing flags on attachment: 80848 Committed r77355: <http://trac.webkit.org/changeset/77355>

All reviewed patches have been landed. Closing bug.

SUCCESS: Build 14507 (r77358) was the first to show failures: set([u'fast/events/pageshow-pagehide-on-back-cached.html', u'fast/history/timed-refresh-in-cached-frame.html']) http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r77400%20(14522)/results.html I think this checkin is to blame.

(In reply to comment #38) > SUCCESS: Build 14507 (r77358) was the first to show failures: set([u'fast/events/pageshow-pagehide-on-back-cached.html', u'fast/history/timed-refresh-in-cached-frame.html']) > > http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r77400%20(14522)/results.html > > I think this checkin is to blame. Filed bug 53648 about this.

This patch broke a test on GTK as well, see bug 53667