Bug 82354 – AssociatedURLLoader does not support Cross Origin Redirects when using Access Control

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Bill Budge]

AssociatedURLLoader should handle CORS requests that redirect. Specifically, it should cancel the load when the redirect fails the CORS check.

[Bill Budge]

Created attachment 134096 [details]
Proposed patch

[Adam Barth]

Comment on attachment 134096 [details]
Proposed patch

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

> Source/WebKit/chromium/tests/AssociatedURLLoaderTest.cpp:471
> -    EXPECT_TRUE(m_willSendRequest);
> +    // CORS requires that the redirect be transparent.
> +    EXPECT_FALSE(m_willSendRequest);

I'm not sure I understand this comment.  This means that willSendRequest isn't called, right?

[Bill Budge]

Yes, that's right. I'll make it clearer.

(In reply to comment #2)
> (From update of attachment 134096 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134096&action=review
> 
> > Source/WebKit/chromium/tests/AssociatedURLLoaderTest.cpp:471
> > -    EXPECT_TRUE(m_willSendRequest);
> > +    // CORS requires that the redirect be transparent.
> > +    EXPECT_FALSE(m_willSendRequest);
> 
> I'm not sure I understand this comment.  This means that willSendRequest isn't called, right?

[Bill Budge]

Created attachment 134165 [details]
Proposed Patch

I worked around the test crash on Mac by not clearing the mocked responses in the test class's TearDown method.

[Adam Barth]

Comment on attachment 134165 [details]
Proposed Patch

Thanks Bill.

[WebKit Review Bot]

Comment on attachment 134165 [details]
Proposed Patch

Clearing flags on attachment: 134165

Committed r112339: <http://trac.webkit.org/changeset/112339>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dirk Pranke]

This change seems to have caused crashes in webkit_unit_tests on snow leopard and lion (chromium)

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.7/builds/2934/steps/webkit_unit_tests/logs/stdio

[Dirk Pranke]

Reverted r112339 for reason:

webkit_unit_tests crashing on chromium mac bots

Committed r112346: <http://trac.webkit.org/changeset/112346>

[Dirk Pranke]

also linux, it appears ...

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux%2032/builds/15415/steps/webkit_unit_tests/logs/stdio

[Bill Budge]

I'll like to re-land this after the fix lands in webkit_support.
http://codereview.chromium.org/9874008/

[Adam Barth]

After that patch lands, would you be willing to update http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/DEPS past that revision so that webkit.org builds see the fix as well?

[Bill Budge]

I will update DEPS before resubmitting.

[Bill Budge]

Created attachment 134382 [details]
Proposed Patch

Fixed WebURLLoaderMockFactory to handle canceled loaders. Bumped DEPS to get fix.

[WebKit Review Bot]

Comment on attachment 134382 [details]
Proposed Patch

Rejecting attachment 134382 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
rce/WebKit/chromium/src/AssociatedURLLoader.cpp
patching file Source/WebKit/chromium/tests/AssociatedURLLoaderTest.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Adam Barth']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/
--mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/12156375

[Bill Budge]

Created attachment 134440 [details]
Proposed Patch

Refreshed patch. DEPS got rolled by someone else.

[WebKit Review Bot]

Comment on attachment 134440 [details]
Proposed Patch

Clearing flags on attachment: 134440

Committed r112485: <http://trac.webkit.org/changeset/112485>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.