| Summary: | REGRESSION (r265908): Crash under Blob::arrayBuffer() / Blob::text() in stress GC | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||
| Component: | Bindings | Assignee: | Chris Dumez <cdumez> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | alecflett, beidson, cdumez, changseok, darin, eric.carlson, esprehn+autocc, ews-watchlist, ggaren, glenn, gyuyoung.kim, hta, jer.noble, jsbell, kangil.han, kondapallykalyan, mifenton, philipj, rniwa, sergio, tommyw, toyoshim, webkit-bug-importer, ysuzuki, yutak | ||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Bug Depends on: | |||||||||||||||||
| Bug Blocks: | 215663 | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Chris Dumez
2020-08-25 18:35:50 PDT
Created attachment 407258 [details]
Patch
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? 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. (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. Created attachment 407265 [details]
Patch
(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. (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. (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) Created attachment 407272 [details]
Patch
Created attachment 407274 [details]
Patch
Created attachment 407296 [details]
Patch
Created attachment 407298 [details]
Patch
Committed r266168: <https://trac.webkit.org/changeset/266168> All reviewed patches have been landed. Closing bug and clearing flags on attachment 407298 [details]. 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?
(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. (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! |