Bug 198534

Summary: Crash when calling XMLHttpRequest.setRequestHeader() in a worker
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebCore Misc.Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, ap, beidson, commit-queue, ews-watchlist, ggaren, rniwa, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
ews-watchlist: commit-queue-
Archive of layout-test-results from ews215 for win-future
none
Archive of layout-test-results from ews101 for mac-highsierra
none
Archive of layout-test-results from ews116 for mac-highsierra
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Archive of layout-test-results from ews105 for mac-highsierra-wk2
none
Patch
none
Patch
none
Patch none

Chris Dumez
Reported 2019-06-04 10:05:29 PDT
Crash when calling XMLHttpRequest.setRequestHeader() in a worker: Thread 8 name: WebCore: Worker Thread 8 Crashed: 0 WebCore 0x00000001a58a0464 WebCore::XMLHttpRequest::setRequestHeader(WTF::String const&, WTF::String const&) + 140 (Settings.h:79) 1 WebCore 0x00000001a58a0444 WebCore::XMLHttpRequest::setRequestHeader(WTF::String const&, WTF::String const&) + 108 (XMLHttpRequest.cpp:148) 2 WebCore 0x00000001a4701f98 WebCore::jsXMLHttpRequestPrototypeFunctionSetRequestHeaderBody(JSC::ExecState*, WebCore::JSXMLHttpRequest*, JSC::ThrowScope&) + 136 (JSXMLHttpRequest.cpp:619) 3 WebCore 0x00000001a46fe964 WebCore::jsXMLHttpRequestPrototypeFunctionSetRequestHeader(JSC::ExecState*) + 152 (JSDOMOperation.h:53) 4 ??? 0x00000007673f3038 0 + 31796965432 5 JavaScriptCore 0x00000001a2d3ac54 llint_entry + 87460 6 JavaScriptCore 0x00000001a2d3ac54 llint_entry + 87460 7 JavaScriptCore 0x00000001a2d3ac54 llint_entry + 87460 8 ??? 0x00000007674cb7ec 0 + 31797852140 9 JavaScriptCore 0x00000001a2d25464 vmEntryToJavaScript + 260 10 JavaScriptCore 0x00000001a327e35c JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 408 (JITCodeInlines.h:38) 11 JavaScriptCore 0x00000001a3576688 JSC::JSPromise::initialize(JSC::ExecState*, JSC::JSGlobalObject*, JSC::JSValue) + 276 (JSPromise.cpp:74) 12 JavaScriptCore 0x00000001a357a774 JSC::constructPromise(JSC::ExecState*) + 196 (JSPromiseConstructor.cpp:117)
Attachments
Patch (4.55 KB, patch)
2019-06-04 10:09 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (8.13 KB, patch)
2019-06-04 11:08 PDT, Chris Dumez 		ews-watchlist: commit-queue-
DetailsFormatted DiffDiff
Archive of layout-test-results from ews215 for win-future (13.60 MB, application/zip)
2019-06-04 12:20 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews101 for mac-highsierra (3.06 MB, application/zip)
2019-06-04 12:22 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews116 for mac-highsierra (2.90 MB, application/zip)
2019-06-04 13:01 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.54 MB, application/zip)
2019-06-04 13:12 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (2.80 MB, application/zip)
2019-06-04 13:28 PDT, EWS Watchlist 		no flags
Details
Patch (4.53 KB, patch)
2019-06-04 15:29 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (4.53 KB, patch)
2019-06-04 15:32 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (4.64 KB, patch)
2019-06-04 15:52 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (9) View All Add attachment proposed patch, testcase, etc.
Chris Dumez
Comment 1 2019-06-04 10:05:45 PDT
<rdar://problem/51393912>
Chris Dumez
Comment 2 2019-06-04 10:09:48 PDT
Created attachment 371297 [details] Patch
Alex Christensen
Comment 3 2019-06-04 10:14:51 PDT
Comment on attachment 371297 [details] Patch You could also check document() instead of scriptExecutionContext()->isDocument()
Chris Dumez
Comment 4 2019-06-04 10:16:29 PDT
(In reply to Alex Christensen from comment #3) > Comment on attachment 371297 [details] > Patch > > You could also check document() instead of > scriptExecutionContext()->isDocument() I don't think so: Document* XMLHttpRequest::document() const { ASSERT(scriptExecutionContext()); return downcast<Document>(scriptExecutionContext()); } It would just do a bad static_cast().
Alexey Proskuryakov
Comment 5 2019-06-04 10:32:18 PDT
Comment on attachment 371297 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371297&action=review > Source/WebCore/ChangeLog:9 > + Make sure the script exxecution context is a Document because calling document() exxecution Is this fully correct behavior, or a hack to make the crash go away? It seems like the setting should probably apply to workers too.
Chris Dumez
Comment 6 2019-06-04 10:45:28 PDT
(In reply to Alexey Proskuryakov from comment #5) > Comment on attachment 371297 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=371297&action=review > > > Source/WebCore/ChangeLog:9 > > + Make sure the script exxecution context is a Document because calling document() > > exxecution > > Is this fully correct behavior, or a hack to make the crash go away? It > seems like the setting should probably apply to workers too. I guess this setting makes sense for workers, I will look into making it work for workers.
Chris Dumez
Comment 7 2019-06-04 11:08:27 PDT
Created attachment 371304 [details] Patch
Chris Dumez
Comment 8 2019-06-04 11:09:23 PDT
Back in review with alternative approach which makes the setting work in workers (and avoids the crash).
Alexey Proskuryakov
Comment 9 2019-06-04 11:23:16 PDT
Comment on attachment 371304 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371304&action=review > Source/WebCore/ChangeLog:9 > + Move allowSettingAnyXHRHeaderFromFileURLs flag from Settings to RuntimeEnabledFeatures so that Now this can't be per document, which seems wrong too.
Chris Dumez
Comment 10 2019-06-04 12:10:47 PDT
(In reply to Alexey Proskuryakov from comment #9) > Comment on attachment 371304 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=371304&action=review > > > Source/WebCore/ChangeLog:9 > > + Move allowSettingAnyXHRHeaderFromFileURLs flag from Settings to RuntimeEnabledFeatures so that > > Now this can't be per document, which seems wrong too. It is per WebProcess which nowadays is pretty much the same thing. I personally we need something even more fine grained.
EWS Watchlist
Comment 11 2019-06-04 12:20:51 PDT
Comment on attachment 371304 [details] Patch Attachment 371304 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12375572 New failing tests: fast/xmlhttprequest/set-dangerous-headers-from-file-when-setting-enabled.html
EWS Watchlist
Comment 12 2019-06-04 12:20:54 PDT
Created attachment 371316 [details] Archive of layout-test-results from ews215 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews215 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
EWS Watchlist
Comment 13 2019-06-04 12:22:13 PDT
Comment on attachment 371304 [details] Patch Attachment 371304 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12375663 New failing tests: fast/xmlhttprequest/set-dangerous-headers-from-file-when-setting-enabled.html
EWS Watchlist
Comment 14 2019-06-04 12:22:15 PDT
Created attachment 371317 [details] Archive of layout-test-results from ews101 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6
Chris Dumez
Comment 15 2019-06-04 12:36:24 PDT
(In reply to Alexey Proskuryakov from comment #9) > Comment on attachment 371304 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=371304&action=review > > > Source/WebCore/ChangeLog:9 > > + Move allowSettingAnyXHRHeaderFromFileURLs flag from Settings to RuntimeEnabledFeatures so that > > Now this can't be per document, which seems wrong too. Ok, how about I land my very first patch that Alex reviewed then? This setting never worked for workers anyway because of the crash.
Chris Dumez
Comment 16 2019-06-04 12:46:55 PDT
And for the record, I believe RuntimeEnabledFeatures is currently our preferred place for flags that need to be checked from multiple threads.
EWS Watchlist
Comment 17 2019-06-04 13:01:55 PDT
Comment on attachment 371304 [details] Patch Attachment 371304 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12375757 New failing tests: fast/xmlhttprequest/set-dangerous-headers-from-file-when-setting-enabled.html
EWS Watchlist
Comment 18 2019-06-04 13:01:57 PDT
Created attachment 371321 [details] Archive of layout-test-results from ews116 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-highsierra Platform: Mac OS X 10.13.6
EWS Watchlist
Comment 19 2019-06-04 13:12:18 PDT
Comment on attachment 371304 [details] Patch Attachment 371304 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12375826 New failing tests: fast/xmlhttprequest/set-dangerous-headers-from-file-when-setting-enabled.html
EWS Watchlist
Comment 20 2019-06-04 13:12:20 PDT
Created attachment 371324 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.5
EWS Watchlist
Comment 21 2019-06-04 13:28:42 PDT
Comment on attachment 371304 [details] Patch Attachment 371304 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12376330 New failing tests: fast/xmlhttprequest/set-dangerous-headers-from-file-when-setting-enabled.html
EWS Watchlist
Comment 22 2019-06-04 13:28:44 PDT
Created attachment 371327 [details] Archive of layout-test-results from ews105 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Chris Dumez
Comment 23 2019-06-04 15:29:51 PDT
Created attachment 371343 [details] Patch
Chris Dumez
Comment 24 2019-06-04 15:32:12 PDT
Created attachment 371344 [details] Patch
Alexey Proskuryakov
Comment 25 2019-06-04 15:51:05 PDT
> Ok, how about I land my very first patch that Alex reviewed then? This setting never worked for workers anyway because of the crash. Seems fine, but I suggest adding a FIXME. > And for the record, I believe RuntimeEnabledFeatures is currently our preferred place for flags that need to be checked from multiple threads. I don't know if any of those need to be per document or per view logically.
Chris Dumez
Comment 26 2019-06-04 15:52:55 PDT
Created attachment 371346 [details] Patch
WebKit Commit Bot
Comment 27 2019-06-04 16:30:55 PDT
The commit-queue encountered the following flaky tests while processing attachment 371346 [details]: fetch/fetch-worker-crash.html bug 187257 (author: youennf@gmail.com) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 28 2019-06-04 16:31:40 PDT
Comment on attachment 371346 [details] Patch Clearing flags on attachment: 371346 Committed r246087: <https://trac.webkit.org/changeset/246087>
WebKit Commit Bot
Comment 29 2019-06-04 16:31:42 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.