Summary: | [Content Extensions] Make blocked async XHR call onerror | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||||||
Component: | WebCore Misc. | Assignee: | Alex Christensen <achristensen> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | ap, beidson, benjamin, buildbot, chris, commit-queue, mcatanzaro, rniwa, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=150577 https://bugzilla.gnome.org/show_bug.cgi?id=754954 |
||||||||||||||||
Attachments: |
|
Description
Alex Christensen
2015-07-07 19:04:35 PDT
Created attachment 256345 [details]
Patch
Comment on attachment 256345 [details] Patch Attachment 256345 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4673705569419264 New failing tests: http/tests/security/mixedContent/insecure-xhr-in-main-frame.html fast/frames/frame-unload-crash.html http/tests/navigation/subframe-pagehide-handler-starts-load.html http/tests/navigation/subframe-pagehide-handler-starts-load2.html Created attachment 256348 [details]
Archive of layout-test-results from ews103 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 256345 [details] Patch Attachment 256345 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4688632728256512 New failing tests: http/tests/security/mixedContent/insecure-xhr-in-main-frame.html fast/frames/frame-unload-crash.html http/tests/navigation/subframe-pagehide-handler-starts-load.html http/tests/navigation/subframe-pagehide-handler-starts-load2.html Created attachment 256349 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 256345 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256345&action=review > Source/WebCore/xml/XMLHttpRequest.cpp:791 > + // ThreadableLoader::create can return null here, for example if we're no longer attached to a page or if a content blocker blocks the load. > // This is true while running onunload handlers. > // FIXME: Maybe we need to be able to send XMLHttpRequests from onunload, <http://bugs.webkit.org/show_bug.cgi?id=10904>. > - // FIXME: Maybe create() can return null for other reasons too? > m_loader = ThreadableLoader::create(scriptExecutionContext(), this, request, options); > if (m_loader) { > // Neither this object nor the JavaScript wrapper should be deleted while > // a request is in progress because we need to keep the listeners alive, > // and they are referenced by the JavaScript wrapper. > setPendingActivity(this); > - } > + } else > + networkError(); I hesitate to believe that synchronously calling networkError here is the right thing to do. I'd wager that it should be called on a 0-delay timer. (In reply to comment #6) > Comment on attachment 256345 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=256345&action=review > > > Source/WebCore/xml/XMLHttpRequest.cpp:791 > > + // ThreadableLoader::create can return null here, for example if we're no longer attached to a page or if a content blocker blocks the load. > > // This is true while running onunload handlers. > > // FIXME: Maybe we need to be able to send XMLHttpRequests from onunload, <http://bugs.webkit.org/show_bug.cgi?id=10904>. > > - // FIXME: Maybe create() can return null for other reasons too? > > m_loader = ThreadableLoader::create(scriptExecutionContext(), this, request, options); > > if (m_loader) { > > // Neither this object nor the JavaScript wrapper should be deleted while > > // a request is in progress because we need to keep the listeners alive, > > // and they are referenced by the JavaScript wrapper. > > setPendingActivity(this); > > - } > > + } else > > + networkError(); > > I hesitate to believe that synchronously calling networkError here is the > right thing to do. > > I'd wager that it should be called on a 0-delay timer. Additionally, as the comment suggests, ThreadableLoader::create can fail for other reasons besides content blocking, such as when there's no page. Is it safe to dispatch events without a page? (In reply to comment #6) > I hesitate to believe that synchronously calling networkError here is the > right thing to do. > > I'd wager that it should be called on a 0-delay timer. networkError calls dispatchErrorEvents Created attachment 256386 [details]
Patch
(In reply to comment #8) > (In reply to comment #6) > > I hesitate to believe that synchronously calling networkError here is the > > right thing to do. > > > > I'd wager that it should be called on a 0-delay timer. > networkError calls dispatchErrorEvents ...which synchronously dispatches. :) I'm curious: is there a reason why the Content Extensions blocking check isn't done during open()? (It looks like we already do that with CSP checks). That'd remove the need for a separate error path for synchronous XHRs vs async ones (see https://bugs.webkit.org/show_bug.cgi?id=146271 for what's currently in place for sync XHR). Bump. :) Created attachment 263028 [details]
Patch
Comment on attachment 263028 [details]
Patch
XHR has onerror.
This patch should make it be called.
Created attachment 263044 [details]
Patch
(In reply to comment #15) > XHR has onerror. > > This patch should make it be called. It was called, it just wasn't in the test of the previous patch. It is in the latest test. Comment on attachment 263044 [details] Patch Clearing flags on attachment: 263044 Committed r191077: <http://trac.webkit.org/changeset/191077> All reviewed patches have been landed. Closing bug. Tested with latest WebKit nightly (r191112) on 10.11 (15A284), and onerror doesn't seem to get called. *** Bug 145717 has been marked as a duplicate of this bug. *** |