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) […]
<rdar://problem/20788804>
Created attachment 334056 [details] Patch
Created attachment 334057 [details] Patch
Created attachment 334062 [details] Patch
Comment on attachment 334062 [details] Patch Clearing flags on attachment: 334062 Committed r228574: <https://trac.webkit.org/changeset/228574>
All reviewed patches have been landed. Closing bug.
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?
(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.
(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.
(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. :)
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.