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
Chris Dumez
2019-06-04 10:05:29 PDT
Created attachment 371297 [details]
Patch
Comment on attachment 371297 [details]
Patch
You could also check document() instead of scriptExecutionContext()->isDocument()
(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(). 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. (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. Created attachment 371304 [details]
Patch
Back in review with alternative approach which makes the setting work in workers (and avoids the crash). 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. (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. 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 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
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 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
(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. And for the record, I believe RuntimeEnabledFeatures is currently our preferred place for flags that need to be checked from multiple threads. 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 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
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 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
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 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
Created attachment 371343 [details]
Patch
Created attachment 371344 [details]
Patch
> 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. Created attachment 371346 [details]
Patch
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. Comment on attachment 371346 [details] Patch Clearing flags on attachment: 371346 Committed r246087: <https://trac.webkit.org/changeset/246087> All reviewed patches have been landed. Closing bug. |