Bug 122592 - REGRESSION(r157210): Crashes in WebCore::ScopedEventQueue::dispatchEvent for platforms using GCC
Summary: REGRESSION(r157210): Crashes in WebCore::ScopedEventQueue::dispatchEvent for ...
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-10 03:05 PDT by Zan Dobersek
Modified: 2013-10-14 02:11 PDT (History)
7 users (show)

See Also:

Attachments
Patch (2.12 KB, patch)
2013-10-10 03:17 PDT, Zan Dobersek 		no flags Details | Formatted Diff | Diff
Patch (2.23 KB, patch)
2013-10-10 05:39 PDT, Zan Dobersek 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2013-10-10 03:05:09 PDT 
REGRESSION(r157210): Crashes in WebCore::ScopedEventQueue::dispatchEvent for platforms using GCC
Comment 1 Zan Dobersek 2013-10-10 03:17:23 PDT 
Created attachment 213863 [details]
Patch
Comment 2 Sergio Villar Senin 2013-10-10 03:35:05 PDT 
Comment on attachment 213863 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +

Looks like something wrong happened here :)

> Source/WebCore/dom/ScopedEventQueue.cpp:83
> +    EventDispatcher::dispatchEvent(node, event);

I think this deserves a comment, because it isn't obvious why we're assigning the pointer first to a local variable.

Or even better, we can assign the event to a local RefPtr and use that in dispatchEvent()
Comment 3 Zan Dobersek 2013-10-10 03:50:30 PDT 
Comment on attachment 213863 [details]
Patch

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

>> Source/WebCore/dom/ScopedEventQueue.cpp:83
>> +    EventDispatcher::dispatchEvent(node, event);
> 
> I think this deserves a comment, because it isn't obvious why we're assigning the pointer first to a local variable.
> 
> Or even better, we can assign the event to a local RefPtr and use that in dispatchEvent()

How about passing in a naked pointer of the event? This won't destruct the original PassRefPtr.

I'm missing support for something like std::forward() here.
Comment 4 Alberto Garcia 2013-10-10 05:35:44 PDT 
(In reply to comment #3)
> How about passing in a naked pointer of the event? This won't
> destruct the original PassRefPtr.

I think it's unclear either way and deserves a comment.
Comment 5 Zan Dobersek 2013-10-10 05:39:47 PDT 
Created attachment 213875 [details]
Patch
Comment 6 Zan Dobersek 2013-10-10 06:28:21 PDT 
Comment on attachment 213875 [details]
Patch

Clearing flags on attachment: 213875

Committed r157219: <http://trac.webkit.org/changeset/157219>
Comment 7 Zan Dobersek 2013-10-10 06:28:27 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 8 Alexey Proskuryakov 2013-10-10 09:46:23 PDT 
I think that the original proposed patch was better, because it avoided reference churn from creating a new PassRefPtr.
Comment 9 Alberto Garcia 2013-10-10 11:58:01 PDT 
(In reply to comment #8)
> I think that the original proposed patch was better, because it
> avoided reference churn from creating a new PassRefPtr.

I also think that it was a bit clearer, the .get() solution makes the
reader wonder what's going on there, while the original one feels
natural.
Comment 10 Zan Dobersek 2013-10-10 13:45:51 PDT 
I'm indifferent here, but I'll upload the adjustment in a separate bug.
Comment 11 Zan Dobersek 2013-10-14 02:11:16 PDT 
(In reply to comment #10)
> I'm indifferent here, but I'll upload the adjustment in a separate bug.

Bug #122742.