Bug 126975 - [XHR] Abort method execution when m_loader->cancel() in internalAbort() caused reentry
Summary: [XHR] Abort method execution when m_loader->cancel() in internalAbort() cause...
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
Keywords: BlinkMergeCandidate
Depends on:
Reported: 2014-01-14 03:24 PST by youenn fablet
Modified: 2014-10-14 10:10 PDT (History)
2 users (show)

See Also:

First patch (9.79 KB, patch)
2014-01-14 05:57 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Improved comments (8.91 KB, patch)
2014-02-05 08:44 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Updated for commit (8.99 KB, patch)
2014-10-14 00:54 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2014-01-14 03:24:08 PST
Consider merging https://chromium.googlesource.com/chromium/blink/+/0d75daf2053631518606ae15daaece701a25b2c4

Calling cancel() on DocumentThreadableLoader may results in calling
window.onload synchronously. If open(), send(), etc. are called on the same
XMLHttpRequest object, it'll be hard to resolve conflict of states without
losing spec conformance. This CL avoids that by just aborting execution of
code for the outer method that calls internalAbort() if it returns false.
Comment 1 youenn fablet 2014-01-14 03:26:30 PST
Note also that Blink test case LayoutTests/http/tests/xmlhttprequest/reentrant-cancel-abort.html added in https://codereview.chromium.org/76133002/ is currently crashing in WebKit.
Comment 2 youenn fablet 2014-01-14 05:57:39 PST
Created attachment 221153 [details]
First patch
Comment 3 Alexey Proskuryakov 2014-01-27 10:56:45 PST
Comment on attachment 221153 [details]
First patch

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

> Source/WebCore/ChangeLog:2
> +2014-01-14  Youenn Fablet  <youennf@gmail.com>
> +        [XHR] Abort method execution when m_loader->cancel() in internalAbort() caused reentry

There should be a blank line here.

> Source/WebCore/xml/XMLHttpRequest.cpp:888
> +    // If window.onload contains open() and send(), m_loader will be set to non 0 value.
> +    // So, we cannot continue the outer open(). In such case, just abort the outer open() by returning false.
> +    // Save this information to a local variable since we are going to drop protection.

It's not clear to me what this "outer open" is, or why returning false from this function will abort anything. In fact, returning false just causes early returns.

> Source/WebCore/xml/XMLHttpRequest.h:199
> -    void internalAbort();
> +    bool internalAbort();

It's not clear what the boolean result means. This should have a comment.

> LayoutTests/ChangeLog:2
> +2014-01-14  Youenn Fablet  <youennf@gmail.com>
> +        [XHR] Abort method execution when m_loader->cancel() in internalAbort() caused reentry

Blank line.

> LayoutTests/ChangeLog:9
> +        Merged reentrant-cancel.html test and changed corresponding expected file
> +        to cope with exception that is now hit in initSend function 

Could you please explain why this exception is being hit? Is this a good thing?

> LayoutTests/http/tests/xmlhttprequest/reentrant-cancel-expected.txt:1
> +CONSOLE MESSAGE: line 26: InvalidStateError: DOM Exception 11: An attempt was made to use an object that is not, or is no longer, usable.

If this exception is now expected, we should probably catch it, and print normally, not leave it unhandled.

> LayoutTests/http/tests/xmlhttprequest/reentrant-cancel-expected.txt:3
> +This tests that when we re-entrantly create and cancel XHRs, we don't try to disconnect the same CachedResourceClient multiple times from its CachedResource. We pass if we don't crash.

Does this test actually test it now?
Comment 4 youenn fablet 2014-02-05 08:44:35 PST
Created attachment 223241 [details]
Improved comments
Comment 5 Alexey Proskuryakov 2014-02-05 15:14:09 PST
Comment on attachment 223241 [details]
Improved comments

This patch didn't have r? flag, but I believe it was meant to.
Comment 6 WebKit Commit Bot 2014-10-13 10:53:02 PDT
Comment on attachment 223241 [details]
Improved comments

Rejecting attachment 223241 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 223241, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.appspot.com/results/4643625384280064
Comment 7 youenn fablet 2014-10-14 00:54:38 PDT
Created attachment 239784 [details]
Updated for commit
Comment 8 WebKit Commit Bot 2014-10-14 10:10:30 PDT
Comment on attachment 239784 [details]
Updated for commit

Clearing flags on attachment: 239784

Committed r174684: <http://trac.webkit.org/changeset/174684>
Comment 9 WebKit Commit Bot 2014-10-14 10:10:33 PDT
All reviewed patches have been landed.  Closing bug.