Bug 28134 - Move Access Control out of XMLHttpRequest
Summary: Move Access Control out of XMLHttpRequest
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Aaron Boodman
URL:
Keywords:
Depends on:
Blocks: 24853
  Show dependency treegraph
 
Reported: 2009-08-09 14:58 PDT by Aaron Boodman
Modified: 2009-08-14 12:25 PDT (History)
5 users (show)

See Also:


Attachments
Proposed patch (55.51 KB, patch)
2009-08-09 15:24 PDT, Aaron Boodman
ap: review-
Details | Formatted Diff | Diff
New patch (54.11 KB, patch)
2009-08-10 17:50 PDT, Aaron Boodman
ap: review+
Details | Formatted Diff | Diff
New patch (55.39 KB, patch)
2009-08-11 17:46 PDT, Aaron Boodman
eric: review+
eric: commit-queue-
Details | Formatted Diff | Diff
New patch (55.39 KB, patch)
2009-08-11 17:46 PDT, Aaron Boodman
no flags Details | Formatted Diff | Diff
Newer patch (55.61 KB, patch)
2009-08-13 16:51 PDT, Aaron Boodman
ap: review+
eric: commit-queue-
Details | Formatted Diff | Diff
One more time patch (56.24 KB, patch)
2009-08-13 20:10 PDT, Aaron Boodman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aaron Boodman 2009-08-09 14:58:36 PDT
This is a good thing to do anyway, just because Access Control is a separate spec that could be used for other things.

But there are also two concrete needs motivating this:

* The CrossOriginAccessControl cache is having to use locks presently, because it is used on multiple threads because of workers. It would be nice to remove those.
* The Chromium project would like to add a callback to allow Chromium to override the default handling of cross-origin XHR, but this cannot be done until we move AC out onto the main thread (see bug 24853)
Comment 1 Aaron Boodman 2009-08-09 15:24:17 PDT
Created attachment 34432 [details]
Proposed patch
Comment 2 Eric Seidel (no email) 2009-08-09 21:26:36 PDT
A rather serious undertaking for your 8th WebKit patch. ;)  I'll take a more serious look tomorrow.  I was slightly surprised this went into DocumentThreadableLoader instead of a new class (given your goals of code re-use), but that will probably make more sense to me when I take a longer look tomorrow.  CC'd Sam who has also worked on XHR.
Comment 3 Aaron Boodman 2009-08-09 22:38:24 PDT
Yeah, I wouldn't have chosen to take on something this large so early in my WebKit career, but I needed it done.

FWIW, Dave Levin offered to do this review for me because he also wants to see this done (for workers), and he also suggested splitting out the access control stuff, which I am happy to do.

But I'm happy to have additional comments. Please keep in mind I'm still wrapping my head around WebKit conventions, so don't assume there is any rhyme or reason to my decisions :).
Comment 4 David Levin 2009-08-09 22:53:35 PDT
I'm pretty familiar with this code so I will look tomorrow unless someone else familiar with the area really wants it.  I was just taking the weekend off.
Comment 5 Alexey Proskuryakov 2009-08-09 23:02:09 PDT
Skimming over the patch, I was surprised by:

+        static void loadResourceSynchronously(ScriptExecutionContext*, const ResourceRequest&, ThreadableLoaderClient&, ThreadableLoaderOptions);

Why is the ThreadableLoaderOptions struct copied, and not passed by reference?

+        // TODO(aa): Is this the right way to notify errors?

Should be "FIXME: ..."

+    // TODO(aa): This doesn't look to me like it will actually follow redirects. Investigate.

Ditto. Yes, redirects don't work yet.
Comment 6 Alexey Proskuryakov 2009-08-10 10:23:57 PDT
Comment on attachment 34432 [details]
Proposed patch

+void DocumentThreadableLoader::loadResourceSynchronously(Document* document, const ResourceRequest& request, ThreadableLoaderClient& client, ThreadableLoaderOptions options)
 {
-    <...>
+    RefPtr<DocumentThreadableLoader> loader = adoptRef(new DocumentThreadableLoader(document, &client, false, request, options));
 }

It's great to re-use async logic here. Some comments though:
- it's worth adding a comment and maybe an assertion (hasOneRef()) to explain that the loader will actually be deleted once this function returns;
- in create(), we have ASSERT(document); please add it here, as well;
- the new sync/async argument should be an enum, not a boolean.

One more question about sharing code - have you checked how auth dialogs behave with sync loads?

+    if (!m_options.forceCrossOriginPreflight && isSimpleCrossOriginAccessRequest(request.httpMethod(), request.httpHeaderFields())) {
+        makeSimpleCrossOriginAccessRequest(request);
+    } else {

No braces around single-line blocks.

+        if (CrossOriginPreflightResultCache::shared().canSkipPreflight(document->securityOrigin()->toString(), request.url(), m_options.allowCredentials, request.httpMethod(), request.httpHeaderFields())) {
+            preflightSuccess();
+        } else

Ditto.

+        loadRequest(request, skipCanLoadCheck, m_options.sendLoadCallbacks, m_options.sniffContent);

It's highly confusing that loadRequest() has two sets of these parameters, one in m_options, and another in explicit arguments. Can anything be done to improve this?

+        preflightRequest.addHTTPHeaderFields(requestHeaderFields);

As an aside, Mozilla debates that this is based on an incorrect reading of the spec, and actual request headers shouldn't be sent with OPTIONS request. Certainly not something to change right now, of course.

+    // FIXME: None of the option parameters are used in the synchronous path. Currently only XHR uses the synchronous path, and it doesn't need these options.

I think this means "none are supported". Could you add assertions to enforce this?

+    // FIXME: This is different than what documents do. Why?
+    if (request.url().isLocalFile())
+      options.sniffContent = true;

I'll defer to Dave to comment on this.

-        
+

I sense evil from check-webkit-style. Not too bad for this particular patch, but fixing style in functions that are not otherwise modified is not helpful.

In general, I really like this patch. Eric's comment about moving the code into a separate class makes sense to me, but that's probably too difficult to do in one step. Also, it's not much practical difference, as ThreadableLoader is used in all the same places we need (or potentially need) access control in.

By the way, why didn't you use bug 24462 for this patch?
Comment 7 David Levin 2009-08-10 11:09:41 PDT
> I sense evil from check-webkit-style.
In this case, that is misplaced.  By default, it only reports errors for lines that were changed.  

For example, when I applied Aaron's patch and ran that tool, it only listed this:

    WebCore/loader/ThreadableLoader.cpp:43:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]

so if he were running the tool, it shouldn't be there.

Also when I ran it on WebCore/loader/ThreadableLoader.cpp alone, it listed the above error plus:
    WebCore/loader/ThreadableLoader.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Comment 8 Aaron Boodman 2009-08-10 11:46:44 PDT
I think I ran the tool incorrect. I did:

check-webkit-style <path-to-file>

I didn't realize there was a way to run it and have in only complain about changed files.
Comment 9 David Levin 2009-08-10 12:06:51 PDT
Re: check-webkit-style
Apparently, it was its evil ways that caused the spacing changes. :)


 +    // FIXME: This is different than what documents do. Why? 
 +    if (request.url().isLocalFile())
 +      options.sniffContent = true;

Good catch. In short, this should be removed now.

Details:
This was added to mirror the logic done in the async XHR case (which was initially added in http://trac.webkit.org/changeset/29370 due to "due to CFNetwork not providing a MIME type for local files").

However, the sniff content was recently removed from xhr async code path in http://trac.webkit.org/changeset/43511 and thus should be removed from here as well.
Comment 10 Aaron Boodman 2009-08-10 17:50:12 PDT
Created attachment 34531 [details]
New patch
Comment 11 Aaron Boodman 2009-08-10 18:19:57 PDT
Ready for another look. Comments inline...

Also, I am getting a test failure on the test cross-origin-no-authorization.html. This failure happens even without my changes, but I have verified that it doesn't happen on the bots.

It looks to me like it is failing because the code in ResourceHandleMac.mm at line ~389 is passing the right value for the flag to make it not send credentials, but the system API is not obeying.

I am wondering if it could be an OS version thing. I am currently running 10.5. 

I am going to try and update my OS but I was wondering if in the meantime anybody could verify whether this system API might have changed in the 10.6/10.7 timeframe.

(In reply to comment #5)
> Skimming over the patch, I was surprised by:
> 
> +        static void loadResourceSynchronously(ScriptExecutionContext*, const
> ResourceRequest&, ThreadableLoaderClient&, ThreadableLoaderOptions);
> 
> Why is the ThreadableLoaderOptions struct copied, and not passed by reference?

No reason. Fixed.

> 
> +        // TODO(aa): Is this the right way to notify errors?
> 
> Should be "FIXME: ..."

Fixed.

> +    // TODO(aa): This doesn't look to me like it will actually follow
> redirects. Investigate.
> 
> Ditto. Yes, redirects don't work yet.

Removed note.(In reply to comment #7)


(In reply to comment #6)
> (From update of attachment 34432 [details])
> +void DocumentThreadableLoader::loadResourceSynchronously(Document* document,
> const ResourceRequest& request, ThreadableLoaderClient& client,
> ThreadableLoaderOptions options)
>  {
> -    <...>
> +    RefPtr<DocumentThreadableLoader> loader = adoptRef(new
> DocumentThreadableLoader(document, &client, false, request, options));
>  }
> 
> It's great to re-use async logic here. Some comments though:
> - it's worth adding a comment and maybe an assertion (hasOneRef()) to explain
> that the loader will actually be deleted once this function returns;

Assertion and comment added.

> - in create(), we have ASSERT(document); please add it here, as well;

We also assert in the constructor, so I took out the one in ::create().

> - the new sync/async argument should be an enum, not a boolean.

Enum added.

> One more question about sharing code - have you checked how auth dialogs behave
> with sync loads?

I'm struggling to even find any test code that uses auth dialogs. Do you know of a good way to test this?

> +    if (!m_options.forceCrossOriginPreflight &&
> isSimpleCrossOriginAccessRequest(request.httpMethod(),
> request.httpHeaderFields())) {
> +        makeSimpleCrossOriginAccessRequest(request);
> +    } else {
> 
> No braces around single-line blocks.
> 
> +        if
> (CrossOriginPreflightResultCache::shared().canSkipPreflight(document->securityOrigin()->toString(),
> request.url(), m_options.allowCredentials, request.httpMethod(),
> request.httpHeaderFields())) {
> +            preflightSuccess();
> +        } else
> 
> Ditto.
> 
> +        loadRequest(request, skipCanLoadCheck, m_options.sendLoadCallbacks,
> m_options.sniffContent);
> 
> It's highly confusing that loadRequest() has two sets of these parameters, one
> in m_options, and another in explicit arguments. Can anything be done to
> improve this?
> 
> +        preflightRequest.addHTTPHeaderFields(requestHeaderFields);
> 
> As an aside, Mozilla debates that this is based on an incorrect reading of the
> spec, and actual request headers shouldn't be sent with OPTIONS request.
> Certainly not something to change right now, of course.
> 
> +    // FIXME: None of the option parameters are used in the synchronous path.
> Currently only XHR uses the synchronous path, and it doesn't need these
> options.
> 
> I think this means "none are supported". Could you add assertions to enforce
> this?

Done.

> I sense evil from check-webkit-style. Not too bad for this particular patch,
> but fixing style in functions that are not otherwise modified is not helpful.

Yeah, I apparently used check-webkit-style incorrectly. Do you mind if I leave the style fixes in?

> In general, I really like this patch. Eric's comment about moving the code into
> a separate class makes sense to me, but that's probably too difficult to do in
> one step. Also, it's not much practical difference, as ThreadableLoader is used
> in all the same places we need (or potentially need) access control in.

I would prefer to leave it in DocumentThreadableLoader for now if you aren't strongly against it.

> By the way, why didn't you use bug 24462 for this patch?

I noticed that bug was already submitted and wasn't sure if that should mean I should create a new one or not.


(In reply to comment #9)
> Re: check-webkit-style
> Apparently, it was its evil ways that caused the spacing changes. :)
> 
> 
>  +    // FIXME: This is different than what documents do. Why? 
>  +    if (request.url().isLocalFile())
>  +      options.sniffContent = true;
> 
> Good catch. In short, this should be removed now.

Ok, I removed it.
Comment 12 Alexey Proskuryakov 2009-08-10 20:25:40 PDT
> I am wondering if it could be an OS version thing. I am currently running 10.5. 

IIRC you need Mac OS X 10.5.7. This test is disabled on Tiger, so it's only on 10.5.x with x < 7 that the test is expected to fail.
Comment 13 Alexey Proskuryakov 2009-08-10 20:30:01 PDT
(In reply to comment #11)

> I'm struggling to even find any test code that uses auth dialogs. Do you know
> of a good way to test this?

No, I don't think there is one currently - except for manual testing.

> Yeah, I apparently used check-webkit-style incorrectly. Do you mind if I leave
> the style fixes in?

That's fine with me.

> > In general, I really like this patch. Eric's comment about moving the code into
> > a separate class makes sense to me, but that's probably too difficult to do in
> > one step. Also, it's not much practical difference, as ThreadableLoader is used
> > in all the same places we need (or potentially need) access control in.
> 
> I would prefer to leave it in DocumentThreadableLoader for now if you aren't
> strongly against it.

Sure.

> > By the way, why didn't you use bug 24462 for this patch?
> 
> I noticed that bug was already submitted and wasn't sure if that should mean I
> should create a new one or not.

I left it open after landing step 1 to continue work there one day. Not a big deal.
Comment 14 Alexey Proskuryakov 2009-08-10 21:15:40 PDT
Comment on attachment 34531 [details]
New patch

> +      ThreadableLoaderOptions() : sendLoadCallbacks(false), sniffContent(false), crossOriginAccessControl(false), allowCredentials(false), forceCrossOriginPreflight(false), allowCrossOriginRedirect(false) {}

We usually put a space between braces in such cases.

> +    // Note: the loader will be deleted as soon as this function exits.

Most comments are also notes :)

r=me, thanks!
Comment 15 Eric Seidel (no email) 2009-08-11 00:09:28 PDT
(In reply to comment #14)
> (From update of attachment 34531 [details])
> > +      ThreadableLoaderOptions() : sendLoadCallbacks(false), sniffContent(false), crossOriginAccessControl(false), allowCredentials(false), forceCrossOriginPreflight(false), allowCrossOriginRedirect(false) {}
> 
> We usually put a space between braces in such cases.

I didn't realize we had a style about such.  Seems we might need to update the guide. :)
Comment 16 Aaron Boodman 2009-08-11 17:46:30 PDT
Created attachment 34620 [details]
New patch

Fix whitespace and comment nits.

ap: If you think this is good can you flip the commit-queue setting so that eric's script will pick it up?
Comment 17 Aaron Boodman 2009-08-11 17:46:48 PDT
Created attachment 34621 [details]
New patch

Fixed whitespace/comment nits.
Comment 18 Alexey Proskuryakov 2009-08-11 20:08:10 PDT
Comment on attachment 34621 [details]
New patch

(In reply to comment #16)
> ap: If you think this is good can you flip the commit-queue setting so that
> eric's script will pick it up?

As they say, flipping the bit is the moral equivalent of committing, and I can't watch the buildbots at the moment, sorry.

By the way, comparing the patch that I reviewed to the latest one shows lots of differences due to order of files. Our svn-create-patch script is supposed to sort changed files to simplify comparing patch revisions - did you use some other tool to make the patches, or did this feature break?

Looks like this latest "New patch" is identical to previous one and is here by accident, obsoleting it to avoid confusion.
Comment 19 Aaron Boodman 2009-08-11 21:00:22 PDT
(In reply to comment #18)
> (From update of attachment 34621 [details])
> (In reply to comment #16)
> > ap: If you think this is good can you flip the commit-queue setting so that
> > eric's script will pick it up?
> 
> As they say, flipping the bit is the moral equivalent of committing, and I
> can't watch the buildbots at the moment, sorry.

Ok, I'll hit someone else up tomorrow.

> By the way, comparing the patch that I reviewed to the latest one shows lots of
> differences due to order of files. Our svn-create-patch script is supposed to
> sort changed files to simplify comparing patch revisions - did you use some
> other tool to make the patches, or did this feature break?

Dunno. I just ran it again with svn-create-patch and it created an identical patch. Maybe I used svn diff the first time, or maybe the script is not working correctly.

> Looks like this latest "New patch" is identical to previous one and is here by
> accident, obsoleting it to avoid confusion.

Thanks.
Comment 20 Eric Seidel (no email) 2009-08-12 11:39:33 PDT
Comment on attachment 34620 [details]
New patch

Forwarding ap's review.
Comment 21 Eric Seidel (no email) 2009-08-12 14:57:47 PDT
Comment on attachment 34620 [details]
New patch

Testing 11049 test cases.
http/tests/workers/worker-importScripts.html -> failed
http/tests/xmlhttprequest/cross-origin-no-authorization.html -> failed

commit queue script hit those errors and I stopped it.
Comment 22 Eric Seidel (no email) 2009-08-12 14:58:20 PDT
I did not investigate the failures enough to know if this patch should be r-'d too.
Comment 23 Aaron Boodman 2009-08-12 15:12:57 PDT
(In reply to comment #21)
> (From update of attachment 34620 [details])
> Testing 11049 test cases.
> http/tests/workers/worker-importScripts.html -> failed
> http/tests/xmlhttprequest/cross-origin-no-authorization.html -> failed
> 
> commit queue script hit those errors and I stopped it.

Thanks, I will investigate. I hit the cross-origin-no-authorization one, but I thought it was due to an incompatibility on the OS I'm using.

I did not see the importScripts one. I will look into that.
Comment 24 Eric Seidel (no email) 2009-08-12 15:36:24 PDT
It's also always possible there was a regression in the tree at the exact moment your patch went through the queue.
Comment 25 Aaron Boodman 2009-08-13 16:51:48 PDT
Created attachment 34797 [details]
Newer patch

Fixed failing layout test. Problem was that I refactored out the calls to setAllowHTTPCookies(false) when XMLHttpRequest.withCredentials was false.
Comment 26 Eric Seidel (no email) 2009-08-13 17:08:04 PDT
Comment on attachment 34797 [details]
Newer patch

Rejecting patch 34797 from commit-queue.  This patch will require manual commit.

['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1
Comment 27 Eric Seidel (no email) 2009-08-13 17:08:30 PDT
http/tests/workers/worker-importScripts.html -> failed
is what caused this failure.
Comment 28 Aaron Boodman 2009-08-13 20:10:40 PDT
Created attachment 34803 [details]
One more time patch

Argh. I was accidentally not running all the tests and missed one failure. It turned out to be rather important, WorkerScriptLoader delegates to DocumentThreadableLoader, and he wants all cross-origin requests to succeed, without any access control being needed.

So I introduced a new enum, CrossOriginRequestPolicy, to go with CrossOriginRedirectPolicy, which callers to DocumentThreadableLoader can use to indicate how they want cross-origin requests to be handled: deny, allow, or use access control.
Comment 29 Alexey Proskuryakov 2009-08-14 10:59:23 PDT
Comment on attachment 34803 [details]
One more time patch

r=me
Comment 30 Eric Seidel (no email) 2009-08-14 11:22:27 PDT
Comment on attachment 34803 [details]
One more time patch

Rejecting patch 34803 from commit-queue.  This patch will require manual commit.

Failed to run "['git', 'svn', 'dcommit']"  exit_code: 1  cwd: None
Comment 31 Eric Seidel (no email) 2009-08-14 11:36:12 PDT
Comment on attachment 34803 [details]
One more time patch

There was a commit just as commit-queue was about to land.  It doesnt' know how to recover from that.  I would add cq+ back, except the tree is red atm, so I'll add it back in a bit (or any commiter can once the tree turns green again).
Comment 32 Eric Seidel (no email) 2009-08-14 12:03:58 PDT
Comment on attachment 34803 [details]
One more time patch

I expect the bots to roll green now.
Comment 33 Eric Seidel (no email) 2009-08-14 12:25:45 PDT
Comment on attachment 34803 [details]
One more time patch

Clearing flags on attachment: 34803

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/loader/DocumentThreadableLoader.cpp
	M	WebCore/loader/DocumentThreadableLoader.h
	M	WebCore/loader/ThreadableLoader.cpp
	M	WebCore/loader/ThreadableLoader.h
	M	WebCore/loader/WorkerThreadableLoader.cpp
	M	WebCore/loader/WorkerThreadableLoader.h
	M	WebCore/platform/CrossThreadCopier.h
	M	WebCore/workers/WorkerScriptLoader.cpp
	M	WebCore/xml/XMLHttpRequest.cpp
	M	WebCore/xml/XMLHttpRequest.h
Committed r47291
	M	WebCore/ChangeLog
	M	WebCore/platform/CrossThreadCopier.h
	M	WebCore/workers/WorkerScriptLoader.cpp
	M	WebCore/xml/XMLHttpRequest.cpp
	M	WebCore/xml/XMLHttpRequest.h
	M	WebCore/loader/WorkerThreadableLoader.cpp
	M	WebCore/loader/ThreadableLoader.cpp
	M	WebCore/loader/WorkerThreadableLoader.h
	M	WebCore/loader/DocumentThreadableLoader.h
	M	WebCore/loader/ThreadableLoader.h
	M	WebCore/loader/DocumentThreadableLoader.cpp
r47291 = cfd042ebd4b8ed42b7a62f963d62dd1c1ff3b6ee (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/47291
Comment 34 Eric Seidel (no email) 2009-08-14 12:25:52 PDT
All reviewed patches have been landed.  Closing bug.