Bug 28134 – Move Access Control out of XMLHttpRequest

• Component: XML
• Status: RESOLVED FIXED

[Aaron Boodman]

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)

[Aaron Boodman]

Created attachment 34432 [details]
Proposed patch

[Eric Seidel (no email)]

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.

[Aaron Boodman]

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 :).

[David Levin]

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.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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?

[David Levin]

> 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]

[Aaron Boodman]

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.

[David Levin]

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.

[Aaron Boodman]

Created attachment 34531 [details]
New patch

[Aaron Boodman]

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.

[Alexey Proskuryakov]

> 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.

[Alexey Proskuryakov]

(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.

[Alexey Proskuryakov]

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!

[Eric Seidel (no email)]

(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. :)

[Aaron Boodman]

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?

[Aaron Boodman]

Created attachment 34621 [details]
New patch

Fixed whitespace/comment nits.

[Alexey Proskuryakov]

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.

[Aaron Boodman]

(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.

[Eric Seidel (no email)]

Comment on attachment 34620 [details]
New patch

Forwarding ap's review.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

I did not investigate the failures enough to know if this patch should be r-'d too.

[Aaron Boodman]

(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.

[Eric Seidel (no email)]

It's also always possible there was a regression in the tree at the exact moment your patch went through the queue.

[Aaron Boodman]

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.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

http/tests/workers/worker-importScripts.html -> failed
is what caused this failure.

[Aaron Boodman]

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.

[Alexey Proskuryakov]

Comment on attachment 34803 [details]
One more time patch

r=me

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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).

[Eric Seidel (no email)]

Comment on attachment 34803 [details]
One more time patch

I expect the bots to roll green now.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.