Bug 160444

Summary: DocumentThreadableLoader should report an error when getting a null CachedResource
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebCore Misc.Assignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, buildbot, cdumez, commit-queue, japhet, rniwa
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 151937    
Attachments:
Description Flags
Patch
buildbot: commit-queue-
Archive of layout-test-results from ews101 for mac-yosemite
none
Archive of layout-test-results from ews105 for mac-yosemite-wk2
none
Rebasing failing test
none
Handling of synchronous didFail in XHR
none
Rebasing tests
none
Rebasing tests
none
Updating setPendingActivity calls none

Description youenn fablet 2016-08-02 01:46:38 PDT
CachedResourceLoader may return a null CachedResource if it detects an error when starting the load.
Typically, these errors are due to access control checks.
For WTR, it may be blocking of non-localhost URLs.

This has the effect of leaving DTL clients waiting for a response that will never come.
For WorkerThreadableLoader, this hits an assertion checking that either the loader is not null or some callbacks have been called.
Comment 1 youenn fablet 2016-08-02 01:51:45 PDT
Created attachment 285086 [details]
Patch
Comment 2 Build Bot 2016-08-02 02:38:28 PDT
Comment on attachment 285086 [details]
Patch

Attachment 285086 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1797520

New failing tests:
fast/frames/frame-unload-crash.html
Comment 3 Build Bot 2016-08-02 02:38:31 PDT
Created attachment 285088 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 4 Build Bot 2016-08-02 02:39:53 PDT
Comment on attachment 285086 [details]
Patch

Attachment 285086 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1797521

New failing tests:
fast/frames/frame-unload-crash.html
Comment 5 Build Bot 2016-08-02 02:39:56 PDT
Created attachment 285089 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 6 youenn fablet 2016-08-02 03:15:12 PDT
Created attachment 285091 [details]
Rebasing failing test
Comment 7 Alex Christensen 2016-08-02 08:42:32 PDT
Comment on attachment 285091 [details]
Rebasing failing test

View in context: https://bugs.webkit.org/attachment.cgi?id=285091&action=review

This doesn't feel like the right thing to do.  Not sure how to describe why.  If an asynchronous load has an error, we usually want to Ref<something> protectedThis(*this); and call the client asynchronously using callOnMainThread.

> Source/WebCore/loader/DocumentThreadableLoader.h:59
> +        bool hasClient() const { return !!m_client; }

This is unused.

> LayoutTests/ChangeLog:9
> +        * http/tests/contentextensions/async-xhr-onerror-expected.txt:

I don't think this test should change behavior.  It already behaves exactly the way we want it to.  See https://bugs.webkit.org/show_bug.cgi?id=146706
Comment 8 youenn fablet 2016-08-02 08:56:09 PDT
(In reply to comment #7)
> Comment on attachment 285091 [details]
> Rebasing failing test
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=285091&action=review
> 
> This doesn't feel like the right thing to do.  Not sure how to describe why.
> If an asynchronous load has an error, we usually want to Ref<something>
> protectedThis(*this); and call the client asynchronously using
> callOnMainThread.

I did a first version of the patch using document.postTask exactly for that purpose, thinking that async might be better for XHR. It does not really matter for fetch because it uses promises anyway. I am not sure it matters for other clients.

But then, in DocumentThreadableLoader constructor, didFail may be called synchronously if trying to do a cross-origin request in SameOrigin fetch mode.
Hence why I went this way, for simplicity and consistency.

That said, there is the issue of scripts that may spot different behaviours in terms of error handling and infer some knowledge, hence the idea to throttle errors.
So, maybe we should change both didFail to an async path.

Wdyt?


> > Source/WebCore/loader/DocumentThreadableLoader.h:59
> > +        bool hasClient() const { return !!m_client; }
> 
> This is unused.

It was used in the first version of the patch using postTask.

> 
> > LayoutTests/ChangeLog:9
> > +        * http/tests/contentextensions/async-xhr-onerror-expected.txt:
> 
> I don't think this test should change behavior.  It already behaves exactly
> the way we want it to.  See https://bugs.webkit.org/show_bug.cgi?id=146706

I'll check this.
Comment 9 youenn fablet 2016-08-03 01:22:34 PDT
Created attachment 285212 [details]
Handling of synchronous didFail in XHR
Comment 10 youenn fablet 2016-08-03 01:36:28 PDT
(In reply to comment #7)
> Comment on attachment 285091 [details]
> Rebasing failing test
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=285091&action=review
> 
> This doesn't feel like the right thing to do.  Not sure how to describe why.
> If an asynchronous load has an error, we usually want to Ref<something>
> protectedThis(*this); and call the client asynchronously using
> callOnMainThread.

Since there are already some didFail synchronous calls, last uploaded patch makes XHR handle them.
This is more consistent with Worker loader code and keeps existing XHR behavior unchanged.

Note though that this departs from the spec since the send() flag should be kept set until actually calling networkError.

> > LayoutTests/ChangeLog:9
> > +        * http/tests/contentextensions/async-xhr-onerror-expected.txt:
> 
> I don't think this test should change behavior.  It already behaves exactly
> the way we want it to.  See https://bugs.webkit.org/show_bug.cgi?id=146706

OK.
To be consistent with XHR spec, the abort call should probably succeed and onabort be called.
Comment 11 youenn fablet 2016-08-04 08:38:37 PDT
Created attachment 285328 [details]
Rebasing tests
Comment 12 youenn fablet 2016-08-04 08:43:53 PDT
Created attachment 285329 [details]
Rebasing tests
Comment 13 Alex Christensen 2016-08-04 10:45:40 PDT
Comment on attachment 285329 [details]
Rebasing tests

View in context: https://bugs.webkit.org/attachment.cgi?id=285329&action=review

> Source/WebCore/xml/XMLHttpRequest.cpp:724
>          setPendingActivity(this);

I think the setPendingActivity calls need to be moved around with this change.  It's hard to follow that the XHR is kept alive for the m_networkErrorTimer.startOneShot in XMLHttpRequest::didFail by this call in a seemingly unrelated location.

> LayoutTests/imported/w3c/web-platform-tests/fetch/api/cors/cors-cookies-expected.txt:8
> +FAIL Include mode: remote cookies are not sent with other remote request promise_test: Unhandled rejection with value: object "TypeError: Type error"

What does it take to make these pass?
Comment 14 youenn fablet 2016-08-04 11:13:01 PDT
> I think the setPendingActivity calls need to be moved around with this
> change.  It's hard to follow that the XHR is kept alive for the
> m_networkErrorTimer.startOneShot in XMLHttpRequest::didFail by this call in
> a seemingly unrelated location.

Right, that will require an additional check that m_loader is null or not but that is clearer.

> > LayoutTests/imported/w3c/web-platform-tests/fetch/api/cors/cors-cookies-expected.txt:8
> > +FAIL Include mode: remote cookies are not sent with other remote request promise_test: Unhandled rejection with value: object "TypeError: Type error"
> 
> What does it take to make these pass?

We would need DRT/WTR to stop blocking www.localhost URLs
We would also need bots to map www.localhost to 127.0.0.1.

Also, this test requires three different domains and we only have localhost and 127.0.0.1 so we cannot change it to use different ports/protocols.
Comment 15 youenn fablet 2016-08-04 11:15:15 PDT
Created attachment 285340 [details]
Updating setPendingActivity calls
Comment 16 WebKit Commit Bot 2016-08-05 00:26:38 PDT
Comment on attachment 285340 [details]
Updating setPendingActivity calls

Clearing flags on attachment: 285340

Committed r204163: <http://trac.webkit.org/changeset/204163>
Comment 17 WebKit Commit Bot 2016-08-05 00:26:45 PDT
All reviewed patches have been landed.  Closing bug.