[XHR] Abort method execution when m_loader->cancel() in internalAbort() caused reentry

• Component: XML
• Status: RESOLVED FIXED

[youenn fablet]

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.

[youenn fablet]

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.

[youenn fablet]

Created attachment 221153 [details]
First patch

[Alexey Proskuryakov]

Comment on attachment 221153 [details]
First patch

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

> Source/WebCore/ChangeLog:2
> [phone]  Youenn Fablet  <[e-mail]>
> +        [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
> [phone]  Youenn Fablet  <[e-mail]>
> +        [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?

[youenn fablet]

Created attachment 223241 [details]
Improved comments

[Alexey Proskuryakov]

Comment on attachment 223241 [details]
Improved comments

This patch didn't have r? flag, but I believe it was meant to.

[WebKit Commit Bot]

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

[youenn fablet]

Created attachment 239784 [details]
Updated for commit

[WebKit Commit Bot]

Comment on attachment 239784 [details]
Updated for commit

Clearing flags on attachment: 239784

Committed r174684: <http://trac.webkit.org/changeset/174684>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.