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
youenn fablet
2016-08-02 01:46:38 PDT
Created attachment 285086 [details]
Patch
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 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 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 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
Created attachment 285091 [details]
Rebasing failing test
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 (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. Created attachment 285212 [details]
Handling of synchronous didFail in XHR
(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. Created attachment 285328 [details]
Rebasing tests
Created attachment 285329 [details]
Rebasing tests
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? > 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. Created attachment 285340 [details]
Updating setPendingActivity calls
Comment on attachment 285340 [details] Updating setPendingActivity calls Clearing flags on attachment: 285340 Committed r204163: <http://trac.webkit.org/changeset/204163> All reviewed patches have been landed. Closing bug. |