Bug 146706 – [Content Extensions] Make blocked async XHR call onerror

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Alex Christensen]

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.

[Alex Christensen]

Created attachment 256345 [details]
Patch

[Build Bot]

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]

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

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

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

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

Created attachment 256386 [details]
Patch

[Brady Eidson]

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

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]

Bump. :)

[Radar WebKit Bug Importer]

<rdar://problem/22925459>

[Alex Christensen]

Created attachment 263028 [details]
Patch

[Brady Eidson]

Comment on attachment 263028 [details]
Patch

XHR has onerror.

This patch should make it be called.

[Alex Christensen]

Created attachment 263044 [details]
Patch

[Alex Christensen]

(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 on attachment 263044 [details]
Patch

Clearing flags on attachment: 263044

Committed r191077: <http://trac.webkit.org/changeset/191077>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Aljoudi]

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

[Michael Catanzaro]

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