Bug 53202 - Async event handlers should not fire within a modal dialog
Summary: Async event handlers should not fire within a modal dialog
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Mihai Parparita
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-26 16:39 PST by Mihai Parparita
Modified: 2011-02-03 01:32 PST (History)
9 users (show)

See Also:


Attachments
Patch (3.76 KB, patch)
2011-01-26 16:43 PST, Mihai Parparita
no flags Details | Formatted Diff | Diff
Patch for landing (3.88 KB, patch)
2011-01-27 22:40 PST, Mihai Parparita
no flags Details | Formatted Diff | Diff
Patch (5.16 KB, patch)
2011-01-28 14:03 PST, Mihai Parparita
no flags Details | Formatted Diff | Diff
Patch (8.03 KB, patch)
2011-01-28 15:13 PST, Mihai Parparita
no flags Details | Formatted Diff | Diff
Patch for landing (8.02 KB, patch)
2011-01-31 15:05 PST, Mihai Parparita
no flags Details | Formatted Diff | Diff
Patch (9.89 KB, patch)
2011-02-01 16:04 PST, Mihai Parparita
no flags Details | Formatted Diff | Diff
Patch for landing (10.14 KB, patch)
2011-02-01 16:13 PST, Mihai Parparita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihai Parparita 2011-01-26 16:39:41 PST
Scroll event callbacks should not fire within a modal dialog
Comment 1 Mihai Parparita 2011-01-26 16:43:38 PST
Created attachment 80263 [details]
Patch
Comment 2 WebKit Commit Bot 2011-01-27 18:07:33 PST
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
Comment 3 Mihai Parparita 2011-01-27 19:32:05 PST
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?
Comment 4 Jeremy Orlow 2011-01-27 19:53:21 PST
+ 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.
Comment 5 Mihai Parparita 2011-01-27 20:43:29 PST
(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.
Comment 6 Mihai Parparita 2011-01-27 22:40:48 PST
Created attachment 80420 [details]
Patch for landing
Comment 7 Alexey Proskuryakov 2011-01-27 23:15:58 PST
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?
Comment 8 Jeremy Orlow 2011-01-28 00:13:29 PST
(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?
Comment 9 Alexey Proskuryakov 2011-01-28 00:35:59 PST
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.
Comment 10 Mihai Parparita 2011-01-28 08:06:36 PST
(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?
Comment 11 Alexey Proskuryakov 2011-01-28 08:39:18 PST
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!
Comment 12 James Robinson 2011-01-28 10:38:58 PST
(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?
Comment 13 Mihai Parparita 2011-01-28 11:14:33 PST
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
Comment 14 Alexey Proskuryakov 2011-01-28 11:26:13 PST
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.
Comment 15 Mihai Parparita 2011-01-28 11:37:17 PST
(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?
Comment 16 Mihai Parparita 2011-01-28 12:22:40 PST
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?
Comment 17 Mihai Parparita 2011-01-28 14:03:24 PST
Created attachment 80494 [details]
Patch
Comment 18 Mihai Parparita 2011-01-28 14:04:59 PST
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 19 Jeremy Orlow 2011-01-28 14:12:24 PST
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
Comment 20 Dmitry Titov 2011-01-28 14:48:13 PST
Just a note: DRT now can test operations under modal dialogs using showModalDialog(). See http://trac.webkit.org/changeset/61599 for more details.
Comment 21 Mihai Parparita 2011-01-28 15:13:24 PST
Created attachment 80504 [details]
Patch
Comment 22 Mihai Parparita 2011-01-28 15:14:57 PST
(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.
Comment 23 Jeremy Orlow 2011-01-28 15:27:04 PST
(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.
Comment 24 Mihai Parparita 2011-01-28 17:41:23 PST
(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 25 James Robinson 2011-01-30 18:56:36 PST
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
Comment 26 Mihai Parparita 2011-01-31 15:05:17 PST
Created attachment 80684 [details]
Patch for landing
Comment 27 Mihai Parparita 2011-01-31 15:05:50 PST
(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 28 WebKit Commit Bot 2011-02-01 01:21:12 PST
Comment on attachment 80684 [details]
Patch for landing

Clearing flags on attachment: 80684

Committed r77230: <http://trac.webkit.org/changeset/77230>
Comment 29 WebKit Commit Bot 2011-02-01 01:21:19 PST
All reviewed patches have been landed.  Closing bug.
Comment 30 Yury Semikhatsky 2011-02-01 04:19:16 PST
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
Comment 31 Mihai Parparita 2011-02-01 16:04:21 PST
Created attachment 80846 [details]
Patch
Comment 32 Mihai Parparita 2011-02-01 16:06:36 PST
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 33 James Robinson 2011-02-01 16:10:09 PST
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.
Comment 34 Mihai Parparita 2011-02-01 16:13:23 PST
Created attachment 80848 [details]
Patch for landing
Comment 35 Mihai Parparita 2011-02-01 16:15:20 PST
(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 36 WebKit Commit Bot 2011-02-01 18:39:43 PST
Comment on attachment 80848 [details]
Patch for landing

Clearing flags on attachment: 80848

Committed r77355: <http://trac.webkit.org/changeset/77355>
Comment 37 WebKit Commit Bot 2011-02-01 18:39:50 PST
All reviewed patches have been landed.  Closing bug.
Comment 38 Eric Seidel (no email) 2011-02-02 14:21:04 PST
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.
Comment 39 Mihai Parparita 2011-02-02 17:05:37 PST
(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.
Comment 40 Philippe Normand 2011-02-03 01:32:09 PST
This patch broke a test on GTK as well, see bug 53667