Bug 110867

Summary: XHR should fire 'abort' event when navigation interrupts a request.
Product: WebKit Reporter: Mike West <mkwst>
Component: WebCore Misc.Assignee: Mike West <mkwst>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, rniwa, roger_fong, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 110007    
Attachments:
Description Flags
Patch
none
Patch for landing
none
Patch for landing none

Description Mike West 2013-02-26 03:50:45 PST 
https://code.google.com/p/chromium/issues/detail?id=162817 and http://bugs.jquery.com/ticket/12964 report that interrupting an XHR request via navigation fires an 'error' event when an 'abort' event should be thrown.
Comment 1 Mike West 2013-02-26 03:54:19 PST 
Created attachment 190255 [details]
Patch
Comment 2 Mike West 2013-02-26 03:55:17 PST 
The trivial layout test seems to show that the behavior is already correct. Asking upstream for either confirmation that the current behavior is correct, or a better test case.
Comment 3 Alexey Proskuryakov 2013-02-26 19:40:30 PST 
Comment on attachment 190255 [details]
Patch

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

> LayoutTests/http/tests/xmlhttprequest/navigation-should-abort-expected.txt:2
> +This is just a minimal page that we navigate to as part of testing back/forward.

This is not accurate. I suggest using a different page, that also says something helpful when opened in a browser manually.

> LayoutTests/http/tests/xmlhttprequest/navigation-should-abort.html:7
> +            testRunner.dumpBackForwardList();

I do not understand why dumping b/f list is helpful.

> LayoutTests/http/tests/xmlhttprequest/navigation-should-abort.html:11
> +        req.open("GET", "/xmlhttprequest/resources/endlessxml.php");

I'd have used a relative path here.

> LayoutTests/http/tests/xmlhttprequest/navigation-should-abort.html:21
> +            testRunner.queueLoad("/navigation/resources/otherpage.html");

Ditto (or maybe just a data: URL).
Comment 4 Mike West 2013-02-27 01:50:15 PST 
(In reply to comment #3)
> (From update of attachment 190255 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190255&action=review
> 
> > LayoutTests/http/tests/xmlhttprequest/navigation-should-abort-expected.txt:2
> > +This is just a minimal page that we navigate to as part of testing back/forward.
> 
> This is not accurate. I suggest using a different page, that also says something helpful when opened in a browser manually.

Swapped out with a `data:` URL as you suggested below.

> > LayoutTests/http/tests/xmlhttprequest/navigation-should-abort.html:7
> > +            testRunner.dumpBackForwardList();
> 
> I do not understand why dumping b/f list is helpful.

The idea was to prove to myself that we're navigating. You're right though, it's likely unnecessary since we get the text of the second page in the expectation file. I'll drop it.

> > LayoutTests/http/tests/xmlhttprequest/navigation-should-abort.html:11
> > +        req.open("GET", "/xmlhttprequest/resources/endlessxml.php");
> 
> I'd have used a relative path here.

Alright.

> > LayoutTests/http/tests/xmlhttprequest/navigation-should-abort.html:21
> > +            testRunner.queueLoad("/navigation/resources/otherpage.html");
> 
> Ditto (or maybe just a data: URL).

Done.

Thanks!
Comment 5 Mike West 2013-02-27 01:50:55 PST 
Created attachment 190468 [details]
Patch for landing
Comment 6 Mike West 2013-02-27 01:51:31 PST 
Comment on attachment 190468 [details]
Patch for landing

CQ- for a formatting error. Bleh.
Comment 7 Mike West 2013-02-27 01:53:48 PST 
Created attachment 190470 [details]
Patch for landing
Comment 8 WebKit Review Bot 2013-02-27 02:55:24 PST 
Comment on attachment 190470 [details]
Patch for landing

Clearing flags on attachment: 190470

Committed r144163: <http://trac.webkit.org/changeset/144163>
Comment 9 WebKit Review Bot 2013-02-27 02:55:28 PST 
All reviewed patches have been landed.  Closing bug.