[webkit-reviews] review denied: [Bug 37781] [XHR] Cross-Origin synchronous request with credential raises NETWORK_ERR : [Attachment 54228] Proposed fix: Clear the credentials from the actual request

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 37781: [XHR] Cross-Origin synchronous request with credential raises
NETWORK_ERR
https://bugs.webkit.org/show_bug.cgi?id=37781

Attachment 54228: Proposed fix: Clear the credentials from the actual request
https://bugs.webkit.org/attachment.cgi?id=54228&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
Good catch! This is a regression, and another fallback from XMLHttpRequest ->
DocumentThreadableLoader code move.

The test is ineffective in DRT - with ToT, it prints PASSED and calls
notifyDone(), so failure printout is lost:

PASSED
FAILED: got exception NETWORK_ERR: XMLHttpRequest Exception 101

Your proposed change modifies the common code path - don't we need a regression
test for async case?

With this change, all code paths in DocumentThreadableLoader now remove
credentials. Rather than duplicating the code, it seems that you could just
move it to DocumentThreadableLoader constructor.

+    // request and response URLs (note: the comparison will fail if any
credential is involved). This isn't a perfect test though,

I don't understand this new comment in parentheses. Could you make it more
specific?