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

Description Alex Christensen 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.
Comment 1 Alex Christensen 2015-07-07 19:07:20 PDT
Created attachment 256345 [details]
Patch
Comment 2 Build Bot 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
Comment 3 Build Bot 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
Comment 4 Build Bot 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
Comment 5 Build Bot 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
Comment 6 Brady Eidson 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.
Comment 7 Brady Eidson 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?
Comment 8 Alex Christensen 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
Comment 9 Alex Christensen 2015-07-08 11:22:11 PDT
Created attachment 256386 [details]
Patch
Comment 10 Brady Eidson 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.  :)
Comment 11 Chris Aljoudi 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).
Comment 12 Chris Aljoudi 2015-09-18 18:54:48 PDT
Bump. :)
Comment 13 Radar WebKit Bug Importer 2015-09-30 16:51:50 PDT
<rdar://problem/22925459>
Comment 14 Alex Christensen 2015-10-13 15:37:53 PDT
Created attachment 263028 [details]
Patch
Comment 15 Brady Eidson 2015-10-13 16:36:21 PDT
Comment on attachment 263028 [details]
Patch

XHR has onerror.

This patch should make it be called.
Comment 16 Alex Christensen 2015-10-13 17:55:56 PDT
Created attachment 263044 [details]
Patch
Comment 17 Alex Christensen 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.
Comment 18 WebKit Commit Bot 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>
Comment 19 WebKit Commit Bot 2015-10-14 17:53:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Chris Aljoudi 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.
Comment 21 Michael Catanzaro 2018-01-08 17:09:32 PST
*** Bug 145717 has been marked as a duplicate of this bug. ***