Bug 122742 - Reintroduce PassRefPtr<Event> copy in ScopedEventQueue::dispatchEvent
Summary: Reintroduce PassRefPtr<Event> copy in ScopedEventQueue::dispatchEvent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-14 01:44 PDT by Zan Dobersek
Modified: 2013-10-14 10:05 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.00 KB, patch)
2013-10-14 02:11 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2013-10-14 01:44:28 PDT
Reintroduce PassRefPtr<Event> copy in ScopedEventQueue::dispatchEvent
Comment 1 Zan Dobersek 2013-10-14 02:11:34 PDT
Created attachment 214136 [details]
Patch
Comment 2 Alberto Garcia 2013-10-14 02:30:27 PDT
lgtm
Comment 3 Zan Dobersek 2013-10-14 08:43:58 PDT
Comment on attachment 214136 [details]
Patch

Clearing flags on attachment: 214136

Committed r157401: <http://trac.webkit.org/changeset/157401>
Comment 4 Zan Dobersek 2013-10-14 08:44:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Darin Adler 2013-10-14 10:05:07 PDT
Comment on attachment 214136 [details]
Patch

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

> Source/WebCore/dom/ScopedEventQueue.cpp:86
> +    // Passing the PassRefPtr<Event> object into the method call creates a new copy and also nullifies
> +    // the original object, which is causing crashes in GCC-compiled code that only after that goes on
> +    // to retrieve the Event's target, calling Event::target() on the now-null PassRefPtr<Event> object.
> +    Node* node = event->target()->toNode();
> +    EventDispatcher::dispatchEvent(node, event);

This is a totally confusing comment.

The bug is no surprise. If we put the node computation into the function call we’d be depending on undefined behavior. But the comment should say something more like this:

    // Put node in local variable to make sure we don’t dereference the event after it's nulled out to pass it in.

The mentions of things like "create a new copy" and "crashes in GCC-compiled code" make the comment just landed confusing and it's *so* long for such a minor issue.