Bug 122592 – REGRESSION(r157210): Crashes in WebCore::ScopedEventQueue::dispatchEvent for platforms using GCC

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

REGRESSION(r157210): Crashes in WebCore::ScopedEventQueue::dispatchEvent for platforms using GCC

[Zan Dobersek]

Created attachment 213863 [details]
Patch

[Sergio Villar Senin]

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

[Zan Dobersek]

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.

[Alberto Garcia]

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

[Zan Dobersek]

Created attachment 213875 [details]
Patch

[Zan Dobersek]

Comment on attachment 213875 [details]
Patch

Clearing flags on attachment: 213875

Committed r157219: <http://trac.webkit.org/changeset/157219>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

I think that the original proposed patch was better, because it avoided reference churn from creating a new PassRefPtr.

[Alberto Garcia]

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

[Zan Dobersek]

I'm indifferent here, but I'll upload the adjustment in a separate bug.

[Zan Dobersek]

(In reply to comment #10)
> I'm indifferent here, but I'll upload the adjustment in a separate bug.

Bug #122742.