Bug 175546 - XHR should only fire an abort event if the cancellation was requested by the client
Summary: XHR should only fire an abort event if the cancellation was requested by the ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 175482
  Show dependency treegraph
 
Reported: 2017-08-14 13:44 PDT by Chris Dumez
Modified: 2017-08-14 18:38 PDT (History)
3 users (show)

See Also:


Attachments
Patch (56.40 KB, patch)
2017-08-14 15:16 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (57.61 KB, patch)
2017-08-14 15:24 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (58.30 KB, patch)
2017-08-14 16:11 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (58.00 KB, patch)
2017-08-14 16:51 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-08-14 13:44:21 PDT
XHR should only fire an abort event if the cancellation was requested by the client, otherwise it should fire an error event.

Specification:
- https://xhr.spec.whatwg.org/#handle-errors
- https://xhr.spec.whatwg.org/#the-abort()-method

Blink matches the specification.
Comment 1 Chris Dumez 2017-08-14 15:16:59 PDT
Created attachment 318067 [details]
Patch
Comment 2 Chris Dumez 2017-08-14 15:24:11 PDT
Created attachment 318070 [details]
Patch
Comment 3 Chris Dumez 2017-08-14 16:11:47 PDT
Created attachment 318079 [details]
Patch
Comment 4 youenn fablet 2017-08-14 16:39:30 PDT
Comment on attachment 318079 [details]
Patch

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

> Source/WebCore/xml/XMLHttpRequest.cpp:1009
> +    // The XHR specification says we should only fire an abort event if the cancelation was requested by the client.

Not sure this comment is needed.

For cancellation errors that are not due to client, they are probably due to some safety security nets.
Would it make sense to long them in the console or is it redundant with web inspector?

> LayoutTests/fast/frames/frame-unload-crash-expected.txt:-2
> -CONSOLE MESSAGE: line 13: XMLHttpRequest cannot load frame-unload-crash-2.html due to access control checks.

This comment might be useful if it is true.
It might indeed be that the below abort is due to access control checks.

> LayoutTests/http/tests/navigation/page-cache-xhr-in-pagehide.html:65
> +    xhr.send();*/

This code is commented out.
Would this test be useful as WPT?
Comment 5 Chris Dumez 2017-08-14 16:50:49 PDT
Comment on attachment 318079 [details]
Patch

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

>> LayoutTests/fast/frames/frame-unload-crash-expected.txt:-2
>> -CONSOLE MESSAGE: line 13: XMLHttpRequest cannot load frame-unload-crash-2.html due to access control checks.
> 
> This comment might be useful if it is true.
> It might indeed be that the below abort is due to access control checks.

The load is cancelled due to a navigation. The console message was inaccurate.

>> LayoutTests/http/tests/navigation/page-cache-xhr-in-pagehide.html:65
>> +    xhr.send();*/
> 
> This code is commented out.
> Would this test be useful as WPT?

Oops. Will remove this commented code.

PageCache is browser-specific so I do not think this should be a WPT.
Comment 6 Chris Dumez 2017-08-14 16:51:32 PDT
Created attachment 318086 [details]
Patch
Comment 7 Chris Dumez 2017-08-14 18:36:10 PDT
Comment on attachment 318086 [details]
Patch

Clearing flags on attachment: 318086

Committed r220731: <http://trac.webkit.org/changeset/220731>
Comment 8 Chris Dumez 2017-08-14 18:36:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2017-08-14 18:38:34 PDT
<rdar://problem/33888764>