Bug 49785

Summary: Move asynchronous event dispatching out of Document
Product: WebKit Reporter: Mihai Parparita <mihai>
Component: DOMAssignee: Mihai Parparita <mihai>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, buildbot, commit-queue, darin, dglazkov, fishd, gns, mjs, ojan, rniwa, sam, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 45631, 45712    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing dglazkov: review+, commit-queue: commit-queue-

Description Mihai Parparita 2010-11-18 22:44:07 PST
Move asynchronous event dispatching out of Document
Comment 1 Mihai Parparita 2010-11-18 22:47:21 PST
Created attachment 74355 [details]
Patch
Comment 2 Mihai Parparita 2010-11-18 22:49:27 PST
This breaks out the EventQueue class out of bug 45631, so that Ryosuke can use it.
Comment 3 Ryosuke Niwa 2010-11-18 23:29:59 PST
Comment on attachment 74355 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74355&action=review

Thanks for doing this!

> WebCore/WebCore.xcodeproj/project.pbxproj:2656
> +		8F67561B1288B17B0047ACA3 /* EventQueue.h in Headers */ = {isa = PBXBuildFile; fileRef = 8F6756191288B17B0047ACA3 /* EventQueue.h */; };

You probably want to make this private.  i.e. settings = {ATTRIBUTES = (Private, ); };

> WebCore/WebCore.xcodeproj/project.pbxproj:18435
> +				8F6756191288B17B0047ACA3 /* EventQueue.h */,
> +				8F67561A1288B17B0047ACA3 /* EventQueue.cpp */,

Please put this in the lexicologically correct location.

> WebCore/WebCore.xcodeproj/project.pbxproj:21372
> +				8F67561B1288B17B0047ACA3 /* EventQueue.h in Headers */,

Ditto.

> WebCore/WebCore.xcodeproj/project.pbxproj:23942
> +				8F67561C1288B17B0047ACA3 /* EventQueue.cpp in Sources */,

Ditto.

> WebCore/dom/EventQueue.cpp:67
> +        if (!eventTarget->toNode()->inDocument())
> +            return;

I'm not sure why you're doing this.  Why shouldn't we fire events when the target node is not in the document?

> WebCore/dom/EventQueue.h:39
> +class EventQueue {

Should this inherit from Noncopyable?
Comment 4 Mihai Parparita 2010-11-19 09:39:16 PST
(In reply to comment #3)
> You probably want to make this private.  i.e. settings = {ATTRIBUTES = (Private, ); };

Why? Despite the name, "private" actually exposes the header outside of the project. The default visibility ("project") is better unless there's a need for EventQueue to be used outside of WebCore. Other headers nearby (e.g. EventContext.h) use "project" visibility too.
 
> > WebCore/WebCore.xcodeproj/project.pbxproj:18435
> > +				8F6756191288B17B0047ACA3 /* EventQueue.h */,
> > +				8F67561A1288B17B0047ACA3 /* EventQueue.cpp */,
> 
> Please put this in the lexicologically correct location.

Done.

> > WebCore/WebCore.xcodeproj/project.pbxproj:21372
> > +				8F67561B1288B17B0047ACA3 /* EventQueue.h in Headers */,
> 
> Ditto.

Done.

> > WebCore/WebCore.xcodeproj/project.pbxproj:23942
> > +				8F67561C1288B17B0047ACA3 /* EventQueue.cpp in Sources */,
> 
> Ditto.

Done.

> 
> > WebCore/dom/EventQueue.cpp:67
> > +        if (!eventTarget->toNode()->inDocument())
> > +            return;
> 
> I'm not sure why you're doing this.  Why shouldn't we fire events when the target node is not in the document?

This was left over from the scrolling-related change, and isn't necessary for this refactoring. Removed.

> 
> > WebCore/dom/EventQueue.h:39
> > +class EventQueue {
> 
> Should this inherit from Noncopyable?

Based on http://trac.webkit.org/changeset/68414, WTF_MAKE_NONCOPYABLE is actually the preferred way of doing this now.
Comment 5 Mihai Parparita 2010-11-19 09:41:51 PST
Created attachment 74399 [details]
Patch
Comment 6 Mihai Parparita 2010-11-19 09:42:15 PST
Ojan may be a good reviewer too.
Comment 7 Ryosuke Niwa 2010-11-19 10:04:14 PST
Comment on attachment 74399 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74399&action=review

> WebCore/dom/EventQueue.cpp:70
> +    else if (eventTarget->toDOMWindow())
> +        eventTarget->toDOMWindow()->dispatchEvent(event, 0);
> +    else
> +        ASSERT_NOT_REACHED();

This "else if" condition is unnecessary since you already asserted
ASSERT(event->target()->toNode() || event->target()->toDOMWindow());
in enqueueEvent.  You can just do:
else {
    ASSERT(event->target()->toDOMWindow());
    eventTarget->toDOMWindow()->dispatchEvent(event, 0);
}

It's not possible for the node or DOMWindow to go away
since the target is stored as RefPtr in Event, right?
Comment 8 Ryosuke Niwa 2010-11-19 10:16:24 PST
(In reply to comment #4)
> (In reply to comment #3)
> > You probably want to make this private.  i.e. settings = {ATTRIBUTES = (Private, ); };
> 
> Why? Despite the name, "private" actually exposes the header outside of the project. The default visibility ("project") is better unless there's a need for EventQueue to be used outside of WebCore. Other headers nearby (e.g. EventContext.h) use "project" visibility too.

Ok, that makes sense.
Comment 9 Darin Adler 2010-11-19 10:19:16 PST
(In reply to comment #7)
> It's not possible for the node or DOMWindow to go away since the target is stored as RefPtr in Event, right?

It all depends what you mean by “go away”. The memory for the object won’t be reclaimed and the object won’t be deleted as long as we hold a reference. But the object can be in a state where it’s no longer reasonable to dispatch events to it.

For example, we don’t want to dispatch events to elements in webpages in the back/forward cache, to name the trickiest example I can think of. Similarly, if we already sent an unload event to a webpage we don’t want to send other events to the elements on the page that are left over from earlier.
Comment 10 Ryosuke Niwa 2010-11-19 10:45:42 PST
(In reply to comment #9)
> It all depends what you mean by “go away”. The memory for the object won’t be reclaimed and the object won’t be deleted as long as we hold a reference. But the object can be in a state where it’s no longer reasonable to dispatch events to it.

Right but calling toDOMWindow() doesn't help, does it?

> For example, we don’t want to dispatch events to elements in webpages in the back/forward cache, to name the trickiest example I can think of. Similarly, if we already sent an unload event to a webpage we don’t want to send other events to the elements on the page that are left over from earlier.

Wait, I thought this won't happen because of ActiveDOMObject?

+ap
Comment 11 Darin Adler 2010-11-19 11:00:40 PST
(In reply to comment #10)
> (In reply to comment #9)
> > It all depends what you mean by “go away”. The memory for the object won’t be reclaimed and the object won’t be deleted as long as we hold a reference. But the object can be in a state where it’s no longer reasonable to dispatch events to it.
> 
> Right but calling toDOMWindow() doesn't help, does it?

I don’t understand this remark. The function toDOMWindow is a good way to convert from an EventTarget* to a DOMWindow*. It has nothing to do with object lifetime; did someone say it does?

> > For example, we don’t want to dispatch events to elements in webpages in the back/forward cache, to name the trickiest example I can think of. Similarly, if we already sent an unload event to a webpage we don’t want to send other events to the elements on the page that are left over from earlier.
> 
> Wait, I thought this won't happen because of ActiveDOMObject?

The ActiveDOMObject mechanism prevents some sorts of activity from occurring while a page is in the back/forward cache. But how does ActiveDOMObject prevent us from dispatching one of these queued events? Clearly it doesn’t.
Comment 12 Ryosuke Niwa 2010-11-19 11:16:08 PST
(In reply to comment #11)
> (In reply to comment #10)
> > Right but calling toDOMWindow() doesn't help, does it?
> 
> I don’t understand this remark. The function toDOMWindow is a good way to convert from an EventTarget* to a DOMWindow*. It has nothing to do with object lifetime; did someone say it does?

Sorry, my point was to say this refactoring wouldn't worsen the problem.  Am I right?

> > > For example, we don’t want to dispatch events to elements in webpages in the back/forward cache, to name the trickiest example I can think of. Similarly, if we already sent an unload event to a webpage we don’t want to send other events to the elements on the page that are left over from earlier.
> >
> > Wait, I thought this won't happen because of ActiveDOMObject?
>
> The ActiveDOMObject mechanism prevents some sorts of activity from occurring while a page is in the back/forward cache. But how does ActiveDOMObject prevent us from dispatching one of these queued events? Clearly it doesn’t.

I see.  Maybe we should check that the document is still attached to a frame?  (i.e. frame() != 0).  But I'm not sure how that helps for DOMWindow.
Comment 13 Ryosuke Niwa 2010-11-19 11:18:30 PST
(In reply to comment #11)
> The ActiveDOMObject mechanism prevents some sorts of activity from occurring while a page is in the back/forward cache. But how does ActiveDOMObject prevent us from dispatching one of these queued events? Clearly it doesn’t.

One more thing.  I was going to suggest that we can add clear() to EventQueue so that we can remove all the pending events when we detach the document and or frame.  What do you think of this idea?
Comment 14 Alexey Proskuryakov 2010-11-19 11:59:07 PST
> For example, we don’t want to dispatch events to elements in webpages in the back/forward cache

This depends on event - for example, async DOM mutation events almost certainly should keep the document object alive and work regardless of whether the document is in frame, or ever was.
Comment 15 Darin Adler 2010-11-19 12:02:15 PST
(In reply to comment #14)
> > For example, we don’t want to dispatch events to elements in webpages in the back/forward cache
> 
> This depends on event - for example, async DOM mutation events almost certainly should keep the document object alive and work regardless of whether the document is in frame, or ever was.

Keeping a document alive is fine, but in the back/forward cache case the document is in suspended animation. I think it would be bad to run any JavaScript at all when the document is in that state. But I could be wrong.
Comment 16 Alexey Proskuryakov 2010-11-19 12:13:06 PST
I agree that running JS in a context of a document that's in b/f cache is unacceptable. But event dispatch is different - a handler can be in another context. E.g.

var doc = frames[0].document;
doc.documentElement.addEventListener("AsyncDOMMutation", function() { alert("ha-ha!") }, true);
frames[0].navigateAway();
doc.documentElement.appendElement(...);

There is no reason for the alert to not be displayed. JSEventListener::handleEvent() takes care of not running scripts in documents that are in b/f cache or frameless.
Comment 17 Ryosuke Niwa 2010-11-22 15:14:01 PST
Could someone review this patch?  There are two bugs depending on this change.
Comment 18 Ryosuke Niwa 2010-11-22 15:19:01 PST
Could someone review this patch?  There are two bugs depending on this change.
Comment 19 Mihai Parparita 2010-12-06 13:05:05 PST
Given Alexey's clarifications in comment 16 about event dispatch and the b/f cache, are there any other concerns with this patch?
Comment 20 Dimitri Glazkov (Google) 2010-12-14 09:16:24 PST
Comment on attachment 74399 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74399&action=review

Looks ok overall, except for the win build fix mentioned.

> WebCore/WebCore.vcproj/WebCore.vcproj:42430
> +				RelativePath="..\dom\EventQueue.cpp"

I think you also need to add this file to DOMAllInOne for it to build on Win.
Comment 21 Mihai Parparita 2010-12-14 14:08:42 PST
Created attachment 76568 [details]
Patch
Comment 22 Mihai Parparita 2010-12-14 14:09:07 PST
(In reply to comment #20)
> I think you also need to add this file to DOMAllInOne for it to build on Win.

Fixed.
Comment 23 WebKit Review Bot 2010-12-14 14:22:02 PST
Attachment 76568 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6948114
Comment 24 Mihai Parparita 2010-12-14 14:23:42 PST
Comment on attachment 76568 [details]
Patch

Looking at Chromium build issue.
Comment 25 WebKit Commit Bot 2010-12-14 14:24:12 PST
Comment on attachment 76568 [details]
Patch

Rejecting attachment 76568 [details] from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'apply-attachment', '--non-interactive', 76568]" exit_code: 2
Last 500 characters of output:
project.pbxproj
patching file WebCore/dom/DOMAllInOne.cpp
patching file WebCore/dom/Document.cpp
patching file WebCore/dom/Document.h
patching file WebCore/dom/EventQueue.cpp
patching file WebCore/dom/EventQueue.h
patching file WebCore/storage/StorageEventDispatcher.cpp
patching file WebKit/chromium/ChangeLog
patching file WebKit/chromium/src/StorageAreaProxy.cpp

Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Dimitri Glazkov', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/6992126
Comment 26 Early Warning System Bot 2010-12-14 14:27:24 PST
Attachment 76568 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7023131
Comment 27 WebKit Review Bot 2010-12-14 14:32:14 PST
Attachment 76568 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7073001
Comment 28 Mihai Parparita 2010-12-14 14:41:37 PST
Created attachment 76574 [details]
Patch for landing
Comment 29 WebKit Commit Bot 2010-12-14 14:43:19 PST
Comment on attachment 76574 [details]
Patch for landing

Rejecting attachment 76574 [details] from commit-queue.

Failed to run "['/Projects/WebKit/WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--non-interactive', 76574]" exit_code: 2
Last 500 characters of output:
ing file WebCore/WebCore.xcodeproj/project.pbxproj
patching file WebCore/dom/DOMAllInOne.cpp
patching file WebCore/dom/Document.cpp
patching file WebCore/dom/Document.h
patching file WebCore/dom/EventQueue.cpp
patching file WebCore/dom/EventQueue.h
patching file WebCore/storage/StorageEventDispatcher.cpp
patching file WebKit/chromium/ChangeLog
patching file WebKit/chromium/src/StorageAreaProxy.cpp

Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/7007113
Comment 30 Mihai Parparita 2010-12-14 14:46:42 PST
(In reply to comment #24)
> (From update of attachment 76568 [details])
> Looking at Chromium build issue.

The EventQueue forward declaration in Document.h got dropped due a merge conflict, hence the build issues (with all platforms). New patch should build correctly.

Will land manually due to the cq complaining about merge issues.
Comment 31 Mihai Parparita 2010-12-14 14:52:40 PST
Committed r74062: <http://trac.webkit.org/changeset/74062>
Comment 32 Build Bot 2010-12-14 15:00:29 PST
Attachment 76568 [details] did not build on win:
Build output: http://queues.webkit.org/results/6979140