Move asynchronous event dispatching out of Document

• Component: DOM
• Status: RESOLVED FIXED

[Mihai Parparita]

Move asynchronous event dispatching out of Document

[Mihai Parparita]

Created attachment 74355 [details]
Patch

[Mihai Parparita]

This breaks out the EventQueue class out of bug 45631, so that Ryosuke can use it.

[Ryosuke Niwa]

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?

[Mihai Parparita]

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

[Mihai Parparita]

Created attachment 74399 [details]
Patch

[Mihai Parparita]

Ojan may be a good reviewer too.

[Ryosuke Niwa]

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?

[Ryosuke Niwa]

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

[Darin Adler]

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

[Ryosuke Niwa]

(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

[Darin Adler]

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

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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

[Alexey Proskuryakov]

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

[Darin Adler]

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

[Alexey Proskuryakov]

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.

[Ryosuke Niwa]

Could someone review this patch?  There are two bugs depending on this change.

[Ryosuke Niwa]

Could someone review this patch?  There are two bugs depending on this change.

[Mihai Parparita]

Given Alexey's clarifications in comment 16 about event dispatch and the b/f cache, are there any other concerns with this patch?

[Dimitri Glazkov (Google)]

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.

[Mihai Parparita]

Created attachment 76568 [details]
Patch

[Mihai Parparita]

(In reply to comment #20)
> I think you also need to add this file to DOMAllInOne for it to build on Win.

Fixed.

[WebKit Review Bot]

Attachment 76568 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6948114

[Mihai Parparita]

Comment on attachment 76568 [details]
Patch

Looking at Chromium build issue.

[WebKit Commit Bot]

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

[Early Warning System Bot]

Attachment 76568 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7023131

[WebKit Review Bot]

Attachment 76568 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7073001

[Mihai Parparita]

Created attachment 76574 [details]
Patch for landing

[WebKit Commit Bot]

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

[Mihai Parparita]

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

[Mihai Parparita]

Committed r74062: <http://trac.webkit.org/changeset/74062>

[Build Bot]

Attachment 76568 [details] did not build on win:
Build output: http://queues.webkit.org/results/6979140