Bug 66340 - REGRESSION (r89086): All worker xhr requests trigger preflight requests.
Summary: REGRESSION (r89086): All worker xhr requests trigger preflight requests.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: David Levin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-16 15:30 PDT by David Levin
Modified: 2011-08-22 13:46 PDT (History)
4 users (show)

See Also:


Attachments
Patch (1.90 KB, patch)
2011-08-16 18:14 PDT, David Levin
no flags Details | Formatted Diff | Diff
Patch (39.49 KB, patch)
2011-08-19 13:25 PDT, David Levin
no flags Details | Formatted Diff | Diff
Test for preflight for simple request from web workers. (7.09 KB, patch)
2011-08-22 11:57 PDT, David Levin
darin: review+
levin: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Levin 2011-08-16 15:30:25 PDT
This happens in WebKit nightlies and chromium for both shared and dedicated workers. After http://trac.webkit.org/changeset/89086 (which I helped review), the referer header is set before DocumentThreadableLoader::create is called.  DocumentThreadableLoader::create  checks to see if a preflight request is needed but the referer header isn't listed as a simple header (because it can't be set by the xhr request at all), so this header being present triggers the preflight request. 

Just adding Nate as an fyi.

In progress fix: 
* I'll add "referer" to the whitelist in isOnAccessControlSimpleRequestHeaderWhitelist in Source/WebCore/loader/CrossOriginAccessControl.cpp and all will be happiness.
* Add a simple test for cross origin stuff for workers/shared workers.
Comment 1 Alexey Proskuryakov 2011-08-16 16:11:08 PDT
I guess we can add all headers in XMLHttpRequest::m_forbiddenRequestHeaders to CORS simple request whitelist now, since they can only be set by the engine, not by JS code.
Comment 2 David Levin 2011-08-16 18:14:01 PDT
Created attachment 104136 [details]
Patch
Comment 3 Adam Barth 2011-08-16 18:15:43 PDT
Comment on attachment 104136 [details]
Patch

Yeah, I agree with Alexey.
Comment 4 David Levin 2011-08-16 18:17:05 PDT
I'll add another patch with tests and add the other headers after I land this one.
Comment 5 David Levin 2011-08-16 18:55:12 PDT
Comment on attachment 104136 [details]
Patch

Committed as http://trac.webkit.org/changeset/93188

Keeping bug open to work on the bigger fix.
Comment 6 Per-Erik Brodin 2011-08-16 19:05:07 PDT
(In reply to comment #1)
> I guess we can add all headers in XMLHttpRequest::m_forbiddenRequestHeaders to CORS simple request whitelist now, since they can only be set by the engine, not by JS code.

Another option would be to fix https://bugs.webkit.org/show_bug.cgi?id=63460
Now Referer and Origin will be incorrectly included in Access-Control-Request-Headers whenever there is a preflight.
Comment 7 David Levin 2011-08-19 13:25:04 PDT
Created attachment 104552 [details]
Patch
Comment 8 David Levin 2011-08-22 11:45:21 PDT
Comment on attachment 104552 [details]
Patch

> Despite what I said in comment 1, I'm not sure if it's the best approach to move
> headers that are special cased in XMLHttpRequest to some shared location. It's
> ThreadableLoader client (in this case, XHR) who really knows if JavaScript code
> called one of those methods that added non-engine headers. Many other clients
> simply don't provide any way to specify custom headers, so doing checks on these
> is pointless.

> What do you think?

It seemed reasonable to move these checks over here as you suggested previously. I suspect the added cost is negligible and this doesn't seem like a performance sensitive path.

However, I don't really care to have all of this moved over and mostly did it due to the suggestion :).

So I'll withdraw this patch and do one with only the test code which should help us catch any future regressions.
Comment 9 David Levin 2011-08-22 11:57:06 PDT
Created attachment 104709 [details]
Test for preflight for simple request from web workers.
Comment 10 Darin Adler 2011-08-22 13:10:24 PDT
Comment on attachment 104709 [details]
Test for preflight for simple request from web workers.

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

> LayoutTests/http/tests/xmlhttprequest/workers/resources/access-control-basic-get-fail-non-simple-test.js:19
> +worker.onmessage = function(evt)

Why eve instead of event?
Comment 11 David Levin 2011-08-22 13:12:19 PDT
Comment on attachment 104709 [details]
Test for preflight for simple request from web workers.

I'll change the test to event and submit it.
Comment 12 David Levin 2011-08-22 13:46:32 PDT
Committed (tests) as http://trac.webkit.org/changeset/93536