Bug 182880 - Crash under WebCore::EventTarget::fireEventListeners
Summary: Crash under WebCore::EventTarget::fireEventListeners
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-16 11:47 PST by Chris Dumez
Modified: 2018-02-19 09:22 PST (History)
10 users (show)

See Also:


Attachments
Patch (6.19 KB, patch)
2018-02-16 11:59 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.17 KB, patch)
2018-02-16 12:01 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.17 KB, patch)
2018-02-16 12:50 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2018-02-16 11:47:15 PST
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)
[…]
Comment 1 Chris Dumez 2018-02-16 11:47:29 PST
<rdar://problem/20788804>
Comment 2 Chris Dumez 2018-02-16 11:59:17 PST
Created attachment 334056 [details]
Patch
Comment 3 Chris Dumez 2018-02-16 12:01:48 PST
Created attachment 334057 [details]
Patch
Comment 4 Chris Dumez 2018-02-16 12:50:00 PST
Created attachment 334062 [details]
Patch
Comment 5 Chris Dumez 2018-02-16 13:06:39 PST
Comment on attachment 334062 [details]
Patch

Clearing flags on attachment: 334062

Committed r228574: <https://trac.webkit.org/changeset/228574>
Comment 6 Chris Dumez 2018-02-16 13:06:40 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 David Kilzer (:ddkilzer) 2018-02-16 18:28:11 PST
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?
Comment 8 Chris Dumez 2018-02-16 18:40:22 PST
(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.
Comment 9 Chris Dumez 2018-02-16 18:58:45 PST
(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.
Comment 10 David Kilzer (:ddkilzer) 2018-02-16 20:23:56 PST
(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.  :)
Comment 11 Andy Estes 2018-02-19 09:22:17 PST
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.