Bug 198534 – Crash when calling XMLHttpRequest.setRequestHeader() in a worker

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

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)

[Chris Dumez]

<rdar://problem/51393912>

[Chris Dumez]

Created attachment 371297 [details]
Patch

[Alex Christensen]

Comment on attachment 371297 [details]
Patch

You could also check document() instead of scriptExecutionContext()->isDocument()

[Chris Dumez]

(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 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]

(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]

Created attachment 371304 [details]
Patch

[Chris Dumez]

Back in review with alternative approach which makes the setting work in workers (and avoids the crash).

[Alexey Proskuryakov]

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]

(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 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]

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 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]

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]

(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]

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 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]

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 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]

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 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]

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]

Created attachment 371343 [details]
Patch

[Chris Dumez]

Created attachment 371344 [details]
Patch

[Alexey Proskuryakov]

> 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]

Created attachment 371346 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 371346 [details]:

fetch/fetch-worker-crash.html bug 187257 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 371346 [details]
Patch

Clearing flags on attachment: 371346

Committed r246087: <https://trac.webkit.org/changeset/246087>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.