Bug 151158 - Stop passing a PassRefPtr to dispatchEvent()
Summary: Stop passing a PassRefPtr to dispatchEvent()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-11-11 15:02 PST by Chris Dumez
Modified: 2015-11-11 23:29 PST (History)
4 users (show)

See Also:


Attachments
Patch (113.41 KB, patch)
2015-11-11 15:34 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (114.06 KB, patch)
2015-11-11 15:59 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (114.29 KB, patch)
2015-11-11 16:06 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (115.19 KB, patch)
2015-11-11 16:16 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (115.16 KB, patch)
2015-11-11 19:54 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (120.96 KB, patch)
2015-11-11 21:37 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (121.03 KB, patch)
2015-11-11 21:49 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (121.31 KB, patch)
2015-11-11 21:51 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (121.57 KB, patch)
2015-11-11 21:55 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2015-11-11 15:02:35 PST
Stop passing a PassRefPtr to dispatchEvent() because:
1. PassRefPtr is legacy and should no longer be used
2. We don't actually transfer ownership of the Event to the callee

Pass a C++ reference instead.
Comment 1 Chris Dumez 2015-11-11 15:34:14 PST
Created attachment 265323 [details]
Patch
Comment 2 Chris Dumez 2015-11-11 15:59:17 PST
Created attachment 265325 [details]
Patch
Comment 3 Chris Dumez 2015-11-11 16:06:08 PST
Created attachment 265326 [details]
Patch
Comment 4 Chris Dumez 2015-11-11 16:16:30 PST
Created attachment 265328 [details]
Patch
Comment 5 Chris Dumez 2015-11-11 19:54:32 PST
Created attachment 265354 [details]
Patch
Comment 6 Alex Christensen 2015-11-11 21:10:11 PST
Comment on attachment 265354 [details]
Patch

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

r=me with comments.  This is a huge code improvement and reduces ref thrashing.  I just have a few questions.  Good luck landing this.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:306
> -        dispatchEvent(event.release());
> +        dispatchEvent(*event);

I don't like adding unchecked dereferencing of pointers.  Could we make events a Vector<Ref<Event>>?

> Source/WebCore/Modules/mediastream/RTCDTMFSender.cpp:165
> +        dispatchEvent(*event);

ditto

> Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:324
> +        dispatchEvent(*event);

ditto

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:745
> +        dispatchEvent(*event);

ditto

> Source/WebCore/css/FontLoader.cpp:191
> +        dispatchEvent(*pendingEvents[index]);

ditto

> Source/WebCore/dom/EventTarget.h:127
> +    bool dispatchEvent(Event*, ExceptionCode&); // DOM API

Why is this comment here?

> Source/WebCore/dom/MessagePort.cpp:-165
> -        dispatchEvent(evt.release(), ASSERT_NO_EXCEPTION);

Should we completely remove IgnorableExceptionCode?

> Source/WebCore/editing/Editor.cpp:-855
> -    target->dispatchEvent(event, IGNORE_EXCEPTION);

Same with IGNORE_EXCEPTION.  Why are we getting rid of this?

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:153
> -        dispatchEvent(WTF::move(deferredProgressEvent));
> +        dispatchEvent(*deferredProgressEvent);

Ditto
Comment 7 Chris Dumez 2015-11-11 21:37:08 PST
Created attachment 265361 [details]
Patch
Comment 8 Chris Dumez 2015-11-11 21:47:11 PST
Comment on attachment 265354 [details]
Patch

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

>> Source/WebCore/Modules/mediastream/MediaStream.cpp:306
>> +        dispatchEvent(*event);
> 
> I don't like adding unchecked dereferencing of pointers.  Could we make events a Vector<Ref<Event>>?

Sure. Will do.

>> Source/WebCore/dom/EventTarget.h:127
>> +    bool dispatchEvent(Event*, ExceptionCode&); // DOM API
> 
> Why is this comment here?

Because this is supposed to be called from JS.

>> Source/WebCore/dom/MessagePort.cpp:-165
>> -        dispatchEvent(evt.release(), ASSERT_NO_EXCEPTION);
> 
> Should we completely remove IgnorableExceptionCode?

I am pretty sure there is still a lot of code that uses it.

>> Source/WebCore/editing/Editor.cpp:-855
>> -    target->dispatchEvent(event, IGNORE_EXCEPTION);
> 
> Same with IGNORE_EXCEPTION.  Why are we getting rid of this?

There are 2 dispatchEvent() overloads:
1. dispatchEvent(Event*, ExceptionCode&)
-> Meant to be used by JS bindings. Because the event comes from JS, we do some additional checks on the event and throw an exception if those checks fail. The checks are: null pointer, event is not initialized, event is already being dispatched.
2. dispatchEvent(Event&)
-> Meant to be called by C++ code. It cannot throw as we expect the event argument to be valid.

Previously, some of our C++ code was unnecessarily calling the overload that is meant to be used by JS so I fixed that. I will also add assertions in dispatchEvent(Event&) to make sure the event is initialized and not already being dispatched.

>> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:153
>> +        dispatchEvent(*deferredProgressEvent);
> 
> Ditto

Well, this one has a null check.
Comment 9 Chris Dumez 2015-11-11 21:49:11 PST
Created attachment 265362 [details]
Patch
Comment 10 Chris Dumez 2015-11-11 21:51:03 PST
Created attachment 265363 [details]
Patch
Comment 11 Chris Dumez 2015-11-11 21:55:24 PST
Created attachment 265364 [details]
Patch
Comment 12 Chris Dumez 2015-11-11 23:28:56 PST
Comment on attachment 265364 [details]
Patch

Clearing flags on attachment: 265364

Committed r192354: <http://trac.webkit.org/changeset/192354>
Comment 13 Chris Dumez 2015-11-11 23:29:00 PST
All reviewed patches have been landed.  Closing bug.