Bug 182880 – Crash under WebCore::EventTarget::fireEventListeners

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Chris Dumez]

Crash under WebCore::EventTarget::fireEventListeners:
Thread 0 name:  Dispatch queue: com.apple.main-thread
Thread 0 Crashed ↩:
0   WebCore                       	0x000000018b640544 WebCore::EventTarget::fireEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::DumbPtrTraits<WebCore::RegisteredEventListener> >, 1ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>) + 76 (Document.h:1925)
1   WebCore                       	0x000000018b640540 WebCore::EventTarget::fireEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::DumbPtrTraits<WebCore::RegisteredEventListener> >, 1ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>) + 72 (EventTarget.cpp:258)
2   WebCore                       	0x000000018b63c3a8 WebCore::EventTarget::fireEventListeners(WebCore::Event&) + 596 (EventTarget.cpp:231)
3   WebCore                       	0x000000018b6404dc WebCore::EventTarget::dispatchEvent(WebCore::Event&) + 116 (EventTarget.cpp:190)
4   WebCore                       	0x000000018b352d40 WTF::Function<void ()>::CallableWrapper<WebCore::AudioScheduledSourceNode::finish()::$_0>::call() + 80 (AudioScheduledSourceNode.cpp:171)
[…]

[Chris Dumez]

<rdar://problem/20788804>

[Chris Dumez]

Created attachment 334056 [details]
Patch

[Chris Dumez]

Created attachment 334057 [details]
Patch

[Chris Dumez]

Created attachment 334062 [details]
Patch

[Chris Dumez]

Comment on attachment 334062 [details]
Patch

Clearing flags on attachment: 334062

Committed r228574: <https://trac.webkit.org/changeset/228574>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

Comment on attachment 334062 [details]
Patch

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

> Source/WebCore/Modules/webaudio/AudioScheduledSourceNode.cpp:177
> +    scriptExecutionContext->postTask([this, protectedThis = makeRef(*this)] (auto&) {

Won’t ‘protectedThis’ be an unused lambda?  Why not remove ‘this’ and use ‘protectedThis’ in the body?

[Chris Dumez]

(In reply to David Kilzer (:ddkilzer) from comment #7)
> Comment on attachment 334062 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334062&action=review
> 
> > Source/WebCore/Modules/webaudio/AudioScheduledSourceNode.cpp:177
> > +    scriptExecutionContext->postTask([this, protectedThis = makeRef(*this)] (auto&) {
> 
> Won’t ‘protectedThis’ be an unused lambda?  Why not remove ‘this’ and use
> ‘protectedThis’ in the body?

Oh, does the compiler complain about this now? We have used this pattern in many places (capture this and protectedThis). It usually keeps the code more concise. However, in this case, I had to use an explicit this-> anyway because Gcc sucks.

[Chris Dumez]

(In reply to Chris Dumez from comment #8)
> (In reply to David Kilzer (:ddkilzer) from comment #7)
> > Comment on attachment 334062 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=334062&action=review
> > 
> > > Source/WebCore/Modules/webaudio/AudioScheduledSourceNode.cpp:177
> > > +    scriptExecutionContext->postTask([this, protectedThis = makeRef(*this)] (auto&) {
> > 
> > Won’t ‘protectedThis’ be an unused lambda?  Why not remove ‘this’ and use
> > ‘protectedThis’ in the body?
> 
> Oh, does the compiler complain about this now? We have used this pattern in
> many places (capture this and protectedThis). It usually keeps the code more
> concise. However, in this case, I had to use an explicit this-> anyway
> because Gcc sucks.

I do not see any build failure anywhere so I do not think we need to change anything. protectedThis variables are generally unused.

[David Kilzer (:ddkilzer)]

(In reply to Chris Dumez from comment #9)
> (In reply to Chris Dumez from comment #8)
> > (In reply to David Kilzer (:ddkilzer) from comment #7)
> > > Comment on attachment 334062 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=334062&action=review
> > > 
> > > > Source/WebCore/Modules/webaudio/AudioScheduledSourceNode.cpp:177
> > > > +    scriptExecutionContext->postTask([this, protectedThis = makeRef(*this)] (auto&) {
> > > 
> > > Won’t ‘protectedThis’ be an unused lambda?  Why not remove ‘this’ and use
> > > ‘protectedThis’ in the body?
> > 
> > Oh, does the compiler complain about this now? We have used this pattern in
> > many places (capture this and protectedThis). It usually keeps the code more
> > concise. However, in this case, I had to use an explicit this-> anyway
> > because Gcc sucks.
> 
> I do not see any build failure anywhere so I do not think we need to change
> anything. protectedThis variables are generally unused.

Okay.  Could have sworn this would have triggered such a warning based on the current code (in future releases with newer clang compilers that warn about this), but the buildbots don't lie.  :)

[Andy Estes]

I don't think new clang warns about captured values with non-trivial destructors, because the effects of running the destructor might be how we intend to "use" the captured value. That's definitely the case for protectedThis.