Bug 146706

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:

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	Flags
Patch	none
Archive of layout-test-results from ews103 for mac-mavericks	none
Archive of layout-test-results from ews105 for mac-mavericks-wk2	none
Patch	none
Patch	none
Patch	none

Alex Christensen

Reported 2015-07-07 19:04:35 PDT

Right now there is no indication that anything has happened and no callbacks, which causes asynchronous XHR to behave strangely. Let's call onerror instead.

Attachments
Patch (5.97 KB, patch) 2015-07-07 19:07 PDT, Alex Christensen	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews103 for mac-mavericks (556.11 KB, application/zip) 2015-07-07 19:42 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (598.74 KB, application/zip) 2015-07-07 19:45 PDT, Build Bot	no flags	Details
Patch (5.03 KB, patch) 2015-07-08 11:22 PDT, Alex Christensen	no flags	Details Formatted Diff Diff
Patch (8.02 KB, patch) 2015-10-13 15:37 PDT, Alex Christensen	no flags	Details Formatted Diff Diff
Patch (8.13 KB, patch) 2015-10-13 17:55 PDT, Alex Christensen	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Alex Christensen

Comment 1 2015-07-07 19:07:20 PDT

Created attachment 256345 [details] Patch

Build Bot

Comment 2 2015-07-07 19:42:21 PDT

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

Build Bot

Comment 3 2015-07-07 19:42:24 PDT

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

Build Bot

Comment 4 2015-07-07 19:45:56 PDT

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

Build Bot

Comment 5 2015-07-07 19:45:58 PDT

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

Brady Eidson

Comment 6 2015-07-07 20:39:03 PDT

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.

Brady Eidson

Comment 7 2015-07-07 20:40:16 PDT

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

Alex Christensen

Comment 8 2015-07-08 11:20:59 PDT

(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

Alex Christensen

Comment 9 2015-07-08 11:22:11 PDT

Created attachment 256386 [details] Patch

Brady Eidson

Comment 10 2015-07-08 13:54:36 PDT

(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. :)

Chris Aljoudi

Comment 11 2015-07-21 23:00:40 PDT

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).

Chris Aljoudi

Comment 12 2015-09-18 18:54:48 PDT

Bump. :)

Radar WebKit Bug Importer

Comment 13 2015-09-30 16:51:50 PDT

<rdar://problem/22925459>

Alex Christensen

Comment 14 2015-10-13 15:37:53 PDT

Created attachment 263028 [details] Patch

Brady Eidson

Comment 15 2015-10-13 16:36:21 PDT

Comment on attachment 263028 [details] Patch XHR has onerror. This patch should make it be called.

Alex Christensen

Comment 16 2015-10-13 17:55:56 PDT

Created attachment 263044 [details] Patch

Alex Christensen

Comment 17 2015-10-13 17:58:57 PDT

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

WebKit Commit Bot

Comment 18 2015-10-14 17:52:59 PDT

Comment on attachment 263044 [details] Patch Clearing flags on attachment: 263044 Committed r191077: <http://trac.webkit.org/changeset/191077>

WebKit Commit Bot

Comment 19 2015-10-14 17:53:04 PDT

All reviewed patches have been landed. Closing bug.

Chris Aljoudi

Comment 20 2015-10-15 15:36:54 PDT

Tested with latest WebKit nightly (r191112) on 10.11 (15A284), and onerror doesn't seem to get called.

Michael Catanzaro

Comment 21 2018-01-08 17:09:32 PST

*** Bug 145717 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.