Bug 215832 – REGRESSION (r265908): Crash under Blob::arrayBuffer() / Blob::text() in stress GC

• Component: Bindings
• Status: RESOLVED FIXED

[Chris Dumez]

Crash under Blob::arrayBuffer() / Blob::text() in stress GC:

Thread 20 Crashed:: WebCore: Worker
0   com.apple.JavaScriptCore      	0x000000010450c850 WTFCrash + 16
1   com.apple.WebCore             	0x000000011d9ab1cb WTFCrashWithInfo(int, char const*, char const*, int) + 27
2   com.apple.WebCore             	0x000000011dc64f4d JSC::JSCell::classInfo(JSC::VM&) const + 141
3   com.apple.WebCore             	0x000000011dc636a1 JSC::JSCell::inherits(JSC::VM&, JSC::ClassInfo const*) const + 33
4   com.apple.WebCore             	0x000000011dca4ea1 bool JSC::JSCastingHelpers::inheritsJSTypeImpl<JSC::JSPromise, JSC::JSCell>(JSC::VM&, JSC::JSCell*, JSC::JSTypeRange) + 97
5   com.apple.WebCore             	0x000000011dca4e27 bool JSC::JSCastingHelpers::InheritsTraits<JSC::JSPromise>::inherits<JSC::JSCell>(JSC::VM&, JSC::JSCell*) + 71
6   com.apple.WebCore             	0x000000011dca4dad JSC::JSPromise* JSC::jsDynamicCast<JSC::JSPromise*, JSC::JSCell>(JSC::VM&, JSC::JSCell*) + 29
7   com.apple.WebCore             	0x000000011dca4d49 JSC::JSPromise* JSC::jsDynamicCast<JSC::JSPromise*>(JSC::VM&, JSC::JSValue) + 89
8   com.apple.WebCore             	0x000000011dca4ce5 WebCore::DOMGuarded<JSC::JSPromise>::guarded() const + 69
9   com.apple.WebCore             	0x000000011dca4b35 WebCore::DeferredPromise::deferred() const + 21
10  com.apple.WebCore             	0x000000011ff7a37e WebCore::DeferredPromise::reject(WebCore::Exception, WebCore::RejectAsHandled) + 78
11  com.apple.WebCore             	0x00000001208e9f21 WebCore::Blob::arrayBuffer(WebCore::ScriptExecutionContext&, WTF::Ref<WebCore::DeferredPromise, WTF::DumbPtrTraits<WebCore::DeferredPromise> >&&)::$_9::operator()(std::__1::unique_ptr<WebCore::BlobLoader, std::__1::default_delete<WebCore::BlobLoader> >) + 161
12  com.apple.WebCore             	0x00000001208e9de9 WTF::Detail::CallableWrapper<WebCore::Blob::arrayBuffer(WebCore::ScriptExecutionContext&, WTF::Ref<WebCore::DeferredPromise, WTF::DumbPtrTraits<WebCore::DeferredPromise> >&&)::$_9, void, std::__1::unique_ptr<WebCore::BlobLoader, std::__1::default_delete<WebCore::BlobLoader> > >::call(std::__1::unique_ptr<WebCore::BlobLoader, std::__1::default_delete<WebCore::BlobLoader> >) + 57
13  com.apple.WebCore             	0x00000001208dbf12 WTF::Function<void (std::__1::unique_ptr<WebCore::BlobLoader, std::__1::default_delete<WebCore::BlobLoader> >)>::operator()(std::__1::unique_ptr<WebCore::BlobLoader, std::__1::default_delete<WebCore::BlobLoader> >) const + 178
14  com.apple.WebCore             	0x00000001208dbd07 WTF::CompletionHandler<void (std::__1::unique_ptr<WebCore::BlobLoader, std::__1::default_delete<WebCore::BlobLoader> >)>::operator()(std::__1::unique_ptr<WebCore::BlobLoader, std::__1::default_delete<WebCore::BlobLoader> >) + 279
15  com.apple.WebCore             	0x00000001208dbbdb WebCore::Blob::loadBlob(WebCore::ScriptExecutionContext&, WebCore::FileReaderLoader::ReadType, WTF::CompletionHandler<void (std::__1::unique_ptr<WebCore::BlobLoader, std::__1::default_delete<WebCore::BlobLoader> >)>&&)::$_7::operator()(WebCore::BlobLoader&) + 75
16  com.apple.WebCore             	0x00000001208dbaa1 WTF::Detail::CallableWrapper<WebCore::Blob::loadBlob(WebCore::ScriptExecutionContext&, WebCore::FileReaderLoader::ReadType, WTF::CompletionHandler<void (std::__1::unique_ptr<WebCore::BlobLoader, std::__1::default_delete<WebCore::BlobLoader> >)>&&)::$_7, void, WebCore::BlobLoader&>::call(WebCore::BlobLoader&) + 49
17  com.apple.WebCore             	0x00000001208da9b7 WTF::Function<void (WebCore::BlobLoader&)>::operator()(WebCore::BlobLoader&) const + 151
18  com.apple.WebCore             	0x00000001208da88a WTF::CompletionHandler<void (WebCore::BlobLoader&)>::operator()(WebCore::BlobLoader&) + 266
19  com.apple.WebCore             	0x00000001208d3569 WebCore::BlobLoader::cancel() + 121
20  com.apple.WebCore             	0x00000001208d493c WebCore::Blob::~Blob() + 124
21  com.apple.WebCore             	0x00000001208d4b05 WebCore::Blob::~Blob() + 21
22  com.apple.WebCore             	0x00000001208d4b29 WebCore::Blob::~Blob() + 25

[Chris Dumez]

<rdar://problem/67741677>

[Chris Dumez]

Created attachment 407258 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 407258 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        No new tests, already covered by existing WPT tests that are crashing on stress GC bots.

Can we add a new test that uses GCController to reliably reproduce this issue without stress GC?

> Source/WebCore/dom/DataTransfer.cpp:371
> -FileList& DataTransfer::files() const
> +FileList& DataTransfer::files(ScriptExecutionContext* document) const

I'm a bit confused here. Why pointer? And why do we need to take ScriptExecutionContext instead of Document??

> Source/WebCore/fileapi/Blob.h:64
> +        auto blob = adoptRef(*new Blob(context));
> +        blob->suspendIfNeeded();

Can we do this in the constructor instead like we do elsewhere?

[Chris Dumez]

Comment on attachment 407258 [details]
Patch

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

>> Source/WebCore/dom/DataTransfer.cpp:371
>> +FileList& DataTransfer::files(ScriptExecutionContext* document) const
> 
> I'm a bit confused here. Why pointer? And why do we need to take ScriptExecutionContext instead of Document??

I can make it a Document I believe. I will likely need to cast at one of the call sites, will check.

>> Source/WebCore/fileapi/Blob.h:64
>> +        blob->suspendIfNeeded();
> 
> Can we do this in the constructor instead like we do elsewhere?

No, as I have mentioned in the past. It is not safe to call suspendIfNeeded() inside the constructors. The reason is that suspendIfNeeded() may call suspend() which may ref |this|. This hits the adoption requirement assertion in RefCounted.
Doing it in the factory is the proper and always correct pattern.

[Ryosuke Niwa]

(In reply to Chris Dumez from comment #4)
> Comment on attachment 407258 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407258&action=review
> 
> >> Source/WebCore/dom/DataTransfer.cpp:371
> >> +FileList& DataTransfer::files(ScriptExecutionContext* document) const
> > 
> > I'm a bit confused here. Why pointer? And why do we need to take ScriptExecutionContext instead of Document??
> 
> I can make it a Document I believe. I will likely need to cast at one of the
> call sites, will check.
> 
> >> Source/WebCore/fileapi/Blob.h:64
> >> +        blob->suspendIfNeeded();
> > 
> > Can we do this in the constructor instead like we do elsewhere?
> 
> No, as I have mentioned in the past. It is not safe to call
> suspendIfNeeded() inside the constructors. The reason is that
> suspendIfNeeded() may call suspend() which may ref |this|. This hits the
> adoption requirement assertion in RefCounted.
> Doing it in the factory is the proper and always correct pattern.

Oh, that's right. We need to figure out making this stuff less error prone and confusing somehow.

[Chris Dumez]

Created attachment 407265 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #6)
> Created attachment 407265 [details]
> Patch

Added a layout test.
Updated files() function to take in a Document instead of a ScriptExecutionContext
Build fixes for iOS and GTK.

[Chris Dumez]

(In reply to Chris Dumez from comment #4)
> Comment on attachment 407258 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407258&action=review
> 
> >> Source/WebCore/dom/DataTransfer.cpp:371
> >> +FileList& DataTransfer::files(ScriptExecutionContext* document) const
> > 
> > I'm a bit confused here. Why pointer? And why do we need to take ScriptExecutionContext instead of Document??
> 
> I can make it a Document I believe. I will likely need to cast at one of the
> call sites, will check.
> 
> >> Source/WebCore/fileapi/Blob.h:64
> >> +        blob->suspendIfNeeded();
> > 
> > Can we do this in the constructor instead like we do elsewhere?
> 
> No, as I have mentioned in the past. It is not safe to call
> suspendIfNeeded() inside the constructors. The reason is that
> suspendIfNeeded() may call suspend() which may ref |this|. This hits the
> adoption requirement assertion in RefCounted.
> Doing it in the factory is the proper and always correct pattern.

Yes, I agree it would be nice to find a nicer pattern.

[Chris Dumez]

(In reply to Chris Dumez from comment #8)
> (In reply to Chris Dumez from comment #4)
> > Comment on attachment 407258 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=407258&action=review
> > 
> > >> Source/WebCore/dom/DataTransfer.cpp:371
> > >> +FileList& DataTransfer::files(ScriptExecutionContext* document) const
> > > 
> > > I'm a bit confused here. Why pointer? And why do we need to take ScriptExecutionContext instead of Document??
> > 
> > I can make it a Document I believe. I will likely need to cast at one of the
> > call sites, will check.
> > 
> > >> Source/WebCore/fileapi/Blob.h:64
> > >> +        blob->suspendIfNeeded();
> > > 
> > > Can we do this in the constructor instead like we do elsewhere?
> > 
> > No, as I have mentioned in the past. It is not safe to call
> > suspendIfNeeded() inside the constructors. The reason is that
> > suspendIfNeeded() may call suspend() which may ref |this|. This hits the
> > adoption requirement assertion in RefCounted.
> > Doing it in the factory is the proper and always correct pattern.
> 
> Yes, I agree it would be nice to find a nicer pattern.

Maybe we could relax adoption requirements in ActiveDOMObject()? (not in this patch though)

[Chris Dumez]

Created attachment 407272 [details]
Patch

[Chris Dumez]

Created attachment 407274 [details]
Patch

[Chris Dumez]

Created attachment 407296 [details]
Patch

[Chris Dumez]

Created attachment 407298 [details]
Patch

[EWS]

Committed r266168: <https://trac.webkit.org/changeset/266168>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407298 [details].

[Darin Adler]

Comment on attachment 407298 [details]
Patch

I don’t like all the uses of Document*, RefPtr<Document>, ScriptExecutionContext* in this new code. Can these all really be null?

[Chris Dumez]

(In reply to Darin Adler from comment #15)
> Comment on attachment 407298 [details]
> Patch
> 
> I don’t like all the uses of Document*, RefPtr<Document>,
> ScriptExecutionContext* in this new code. Can these all really be null?

It is allowed/supported by the ActiveDOMObject constructor to pass in null. I had an earlier iteration passing a reference but it required some null checking and promise rejection at some call sites (made the patch bigger and looked riskier).

If you want though, I can upload a follow-up patch to make it a reference. I don't think we'd need that reference patch for the branch though.

[Darin Adler]

(In reply to Chris Dumez from comment #16)
> If you want though, I can upload a follow-up patch to make it a reference. I
> don't think we'd need that reference patch for the branch though.

I’d like to do this if it makes the code better and clearer. I never want to use a pointer just because we "can" take a pointer, but it’s fine when we need to allow null.

If it makes the code more complex and has no real benefit, then please don’t do it!