Bug 160444 – DocumentThreadableLoader should report an error when getting a null CachedResource

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

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.

[youenn fablet]

Created attachment 285086 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[youenn fablet]

Created attachment 285091 [details]
Rebasing failing test

[Alex Christensen]

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

[youenn fablet]

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

[youenn fablet]

Created attachment 285212 [details]
Handling of synchronous didFail in XHR

[youenn fablet]

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

[youenn fablet]

Created attachment 285328 [details]
Rebasing tests

[youenn fablet]

Created attachment 285329 [details]
Rebasing tests

[Alex Christensen]

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?

[youenn fablet]

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

[youenn fablet]

Created attachment 285340 [details]
Updating setPendingActivity calls

[WebKit Commit Bot]

Comment on attachment 285340 [details]
Updating setPendingActivity calls

Clearing flags on attachment: 285340

Committed r204163: <http://trac.webkit.org/changeset/204163>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.