Bug 53202 – Async event handlers should not fire within a modal dialog

• Component: DOM
• Status: RESOLVED FIXED

[Mihai Parparita]

Scroll event callbacks should not fire within a modal dialog

[Mihai Parparita]

Created attachment 80263 [details]
Patch

[WebKit Commit Bot]

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

[Mihai Parparita]

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?

[Jeremy Orlow]

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

[Mihai Parparita]

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

[Mihai Parparita]

Created attachment 80420 [details]
Patch for landing

[Alexey Proskuryakov]

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?

[Jeremy Orlow]

(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?

[Alexey Proskuryakov]

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.

[Mihai Parparita]

(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?

[Alexey Proskuryakov]

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!

[James Robinson]

(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?

[Mihai Parparita]

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

[Alexey Proskuryakov]

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.

[Mihai Parparita]

(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?

[Mihai Parparita]

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?

[Mihai Parparita]

Created attachment 80494 [details]
Patch

[Mihai Parparita]

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

[Jeremy Orlow]

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

[Dmitry Titov]

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

[Mihai Parparita]

Created attachment 80504 [details]
Patch

[Mihai Parparita]

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

[Jeremy Orlow]

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

[Mihai Parparita]

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

[James Robinson]

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

[Mihai Parparita]

Created attachment 80684 [details]
Patch for landing

[Mihai Parparita]

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

[WebKit Commit Bot]

Comment on attachment 80684 [details]
Patch for landing

Clearing flags on attachment: 80684

Committed r77230: <http://trac.webkit.org/changeset/77230>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yury Semikhatsky]

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

[Mihai Parparita]

Created attachment 80846 [details]
Patch

[Mihai Parparita]

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.

[James Robinson]

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.

[Mihai Parparita]

Created attachment 80848 [details]
Patch for landing

[Mihai Parparita]

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

[WebKit Commit Bot]

Comment on attachment 80848 [details]
Patch for landing

Clearing flags on attachment: 80848

Committed r77355: <http://trac.webkit.org/changeset/77355>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

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.

[Mihai Parparita]

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

[Philippe Normand]

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