Bug 53925 - AssociatedURLLoader does not support Cross Origin Requests
Summary: AssociatedURLLoader does not support Cross Origin Requests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Bill Budge
URL:
Keywords:
Depends on: 54196 54287 54313 54489 54688 55680 55863 56432 56479
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-07 10:23 PST by Bill Budge
Modified: 2011-03-16 17:45 PDT (History)
6 users (show)

See Also:


Attachments
Change AssociatedURLLoader to support CORS, using DocumentThreadableLoader (30.69 KB, patch)
2011-02-09 17:37 PST, Bill Budge
levin: review-
Details | Formatted Diff | Diff
AssociatedURLLoader supports CORS using DocumentThreadableLoader (30.40 KB, patch)
2011-02-09 18:07 PST, Bill Budge
no flags Details | Formatted Diff | Diff
Proposed Patch (14.17 KB, patch)
2011-02-25 15:02 PST, Bill Budge
levin: review-
levin: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (13.87 KB, patch)
2011-03-01 15:00 PST, Bill Budge
levin: review-
Details | Formatted Diff | Diff
Proposed Patch (14.04 KB, patch)
2011-03-01 17:02 PST, Bill Budge
levin: review-
Details | Formatted Diff | Diff
Proposed Patch (14.11 KB, patch)
2011-03-02 11:17 PST, Bill Budge
no flags Details | Formatted Diff | Diff
Proposed Patch (14.12 KB, patch)
2011-03-04 18:39 PST, Bill Budge
no flags Details | Formatted Diff | Diff
Proposed Patch (14.87 KB, patch)
2011-03-10 10:20 PST, Bill Budge
no flags Details | Formatted Diff | Diff
Proposed Patch (15.36 KB, patch)
2011-03-11 13:45 PST, Bill Budge
no flags Details | Formatted Diff | Diff
Proposed Patch (15.62 KB, patch)
2011-03-11 14:36 PST, Bill Budge
no flags Details | Formatted Diff | Diff
Proposed Patch (15.80 KB, patch)
2011-03-16 17:01 PDT, Bill Budge
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bill Budge 2011-02-07 10:23:30 PST
The AssociatedURLLoader (/WebKit/Source/WebKit/Chromium/src/AssociatedURLLoader) doesn't support Cross Origin Requests.
Comment 1 Bill Budge 2011-02-09 17:37:31 PST
Created attachment 81901 [details]
Change AssociatedURLLoader to support CORS, using DocumentThreadableLoader
Comment 2 WebKit Review Bot 2011-02-09 17:39:58 PST
Attachment 81901 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Last 3072 characters of output:
 so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/AssociatedURLLoader.cpp:88:  The parameter name "loader" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/AssociatedURLLoader.cpp:89:  The parameter name "loader" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/AssociatedURLLoader.cpp:90:  The parameter name "loader" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/AssociatedURLLoader.cpp:91:  The parameter name "loader" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/AssociatedURLLoader.cpp:92:  The parameter name "loader" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/AssociatedURLLoader.cpp:93:  The parameter name "loader" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/AssociatedURLLoader.cpp:93:  The parameter name "error" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/AssociatedURLLoader.cpp:97:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebKit/chromium/src/AssociatedURLLoader.cpp:121:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebKit/chromium/src/AssociatedURLLoader.cpp:174:  web_error is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit/chromium/src/AssociatedURLLoader.cpp:202:  webcore_request is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit/chromium/src/AssociatedURLLoader.cpp:203:  webcore_document is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit/chromium/src/AssociatedURLLoader.cpp:216:  webcore_request is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit/chromium/src/AssociatedURLLoader.cpp:217:  webcore_document is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit/chromium/src/AssociatedURLLoader.h:58:  One space before end of line comments  [whitespace/comments] [5]
Source/WebKit/chromium/src/AssociatedURLLoader.h:59:  One space before end of line comments  [whitespace/comments] [5]
Source/WebKit/chromium/src/AssociatedURLLoader.h:66:  The parameter name "frame" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/AssociatedURLLoader.h:67:  The parameter name "frame" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/AssociatedURLLoader.h:67:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 26 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 WebKit Review Bot 2011-02-09 17:51:11 PST
Attachment 81901 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7869107
Comment 4 David Levin 2011-02-09 18:06:23 PST
Comment on attachment 81901 [details]
Change AssociatedURLLoader to support CORS, using DocumentThreadableLoader

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

I tend to do two passes. In my first pass, I catch surface level things. In my second pass I try to understand the whole patch and make sense of it.

These are the comments from my first pass but there are so many things that it makes it harder for me to go more in depth right now, so I'm r- because I believe that they can be quickly addressed which will get rid of a lot of comment clutter in this review.

Also it appears that this patch is doing several things at once. It may speed the process of landing parts and improve the quality of review to try to keep these items separate if possible (and make my "second pass" easier). For example, it looks like m_downloadFilePath is pushed downward and that could be a patch by itself. (At this point I haven't yet figured out why that is needed for this change because I haven't really started my "second pass".)

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

There either need to be tests or an explanation of why tests aren't needed (or possible), but regardless this line should disappear.

> Source/WebCore/ChangeLog:11
> +        (WebCore::DocumentThreadableLoader::DocumentThreadableLoader):

This is where an explanation of why a change was done should go.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:-301
> -    actualRequest->setHTTPOrigin(m_document->securityOrigin()->toString());

Why is this line being deleted?

> Source/WebCore/loader/DocumentThreadableLoader.h:58
> +        virtual void setDefersLoading(bool value);

value adds no information so it shouldn't be here.

> Source/WebCore/platform/network/chromium/ResourceRequest.h:124
> +        bool m_downloadToFile;

Good call adding it here, but this structure is used to copy ResourceRequestData and I don't see that code being modified in this change.

> Source/WebCore/platform/network/chromium/ResourceResponse.h:-100
> -        void setSocketAddress(const String& value) { m_socketAddress = value; }

Why is this being deleted?

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:37
> +

No blank line needed here.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:80
> +// SubresourceLoaderClient, which is close to them WebURLLoaderClient.

typo? "close to them WebURLLoaderClient"

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:85
> +    Loader(AssociatedURLLoader* owner, Document* document, BlockingBehavior blockingBehavior, const ResourceRequest& request, const ThreadableLoaderOptions& options);

This constructor should be private (so that the create method is the only way to construct this).

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:87
> +    virtual void AssociatedURLLoader::Loader::willSendRequest(SubresourceLoader* loader, ResourceRequest& request, const ResourceResponse& redirectResponse);

s/AssociatedURLLoader::Loader::willSendRequest/willSendRequest/

The same for other functions declared here.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:96
> +    class DummyClient : public ThreadableLoaderClient

Why not just use ThreadableLoaderClient?

>> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:121
>> +    : DocumentThreadableLoader(document, &m_dummyClient, blockingBehavior, request, options), m_owner(owner) {
> 
> Place brace on its own line for function definitions.  [whitespace/braces] [4]

The initializers should be on separate lines as well.

> Source/WebKit/chromium/src/WebURLResponse.cpp:383
> +  m_private->m_resourceResponse->setDownloadFilePath(downloadFilePath.data());

4 space indent.
Comment 5 Bill Budge 2011-02-09 18:07:16 PST
Created attachment 81907 [details]
AssociatedURLLoader supports CORS using DocumentThreadableLoader
Comment 6 WebKit Review Bot 2011-02-09 18:19:37 PST
Attachment 81907 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7870095
Comment 7 David Levin 2011-02-10 13:32:03 PST
Comment on attachment 81907 [details]
AssociatedURLLoader supports CORS using DocumentThreadableLoader

This particular patch is obsolete. It already has been broken down into at least one other change.
Comment 8 Bill Budge 2011-02-17 16:08:48 PST
I needed another method on ThreadableLoaderClient
Comment 9 Bill Budge 2011-02-17 16:17:08 PST
I needed another method on ThreadableLoaderClient
Comment 10 Bill Budge 2011-02-25 15:02:32 PST
Created attachment 83884 [details]
Proposed Patch

I'm not sure if this needs tests or not.
Comment 11 David Levin 2011-02-25 16:21:04 PST
Comment on attachment 83884 [details]
Proposed Patch

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

> Source/WebKit/chromium/ChangeLog:11
> +        (WTF::ConvertOptions):

Please fix this. (It isn't WTF::ConvertOptions. The script that generates this got confused.)

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

Typically in WebKit land, additional years just get tacked on to the end as opposed to replacing the other year. (I know this is different from how chromium does things.)

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:37
> +

No need for a blank line here.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:61
> +    switch (options.crossOriginRequestPolicy) {

You could add compile asserts to verify that these enums have the same values.
Then replace the switch with a static cast.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:177
> +    m_clientAdapter = PassOwnPtr<ClientAdapter>(new ClientAdapter(this, m_client, request.downloadToFile()));

Two things look wrong here:
1. adoptPtr should be used instead of PassOwnPtr.
2. Really, the typical pattern is to make the constructor private and and expose a static create method in ClientAdapter which returns PassOwnPtr<ClientAdapter> and does
return adoptPtr(new ClientAdapter(...));
internally.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:192
> +    m_clientAdapter = PassOwnPtr<ClientAdapter>(new ClientAdapter(this, m_client, request.downloadToFile()));

This code looks the same as loadSynchronously. Can they share more code?

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:199
> +       m_loader->cancel();

Indentation appears not to be 4 spaces.

> Source/WebKit/chromium/src/AssociatedURLLoader.h:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

Same thing about the copyright years.

> Source/WebKit/chromium/src/AssociatedURLLoader.h:42
> +class ThreadableLoaderClient;

ThreadableLoaderClient isn't used in this header (so remove the fwd decl.

> Source/WebKit/chromium/src/AssociatedURLLoader.h:44
> +} // namespace WebCore

No need for the ending comment in WebKit (and in this case I would omit it since the namespace is so small).

> Source/WebKit/chromium/src/AssociatedURLLoader.h:71
> +class AssociatedURLLoader : public WebURLLoader {

I would add a WTF_MAKE_NONCOPYABLE(AssociatedURLLoader); here (from wtf/Noncopyable.h).

> Source/WebKit/chromium/src/AssociatedURLLoader.h:87
> +    class ClientAdapter;

This feels out of place. I would put it after private.
Comment 12 Bill Budge 2011-03-01 15:00:35 PST
Created attachment 84301 [details]
Proposed Patch

2 questions for David:
1) I noticed an AssertMatchingEnums file for syncing enums. It seemed better to me to do this in AssociatedURLLoader.cpp. Is that OK?
2) AssociatedURLLoader::loadSynchronously probably isn't working after this change, since its response, error, and data objects aren't passed through.
Comment 13 David Levin 2011-03-01 15:02:58 PST
(In reply to comment #12)
> Created an attachment (id=84301) [details]
> Proposed Patch
> 
> 2 questions for David:
> 1) I noticed an AssertMatchingEnums file for syncing enums. It seemed better to me to do this in AssociatedURLLoader.cpp. Is that OK?

Sounds even better (than what I said).

> 2) AssociatedURLLoader::loadSynchronously probably isn't working after this change, since its response, error, and data objects aren't passed through.

What's the question? :)
Comment 14 Bill Budge 2011-03-01 15:07:19 PST
I guess my question was "Do you think it's important for the synchronous case to work now (or ever) and can it be added later, or should I connect this through WebCore as I did for downloadToFile stuff?"
Comment 15 David Levin 2011-03-01 15:12:01 PST
Comment on attachment 84301 [details]
Proposed Patch

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

A few comments.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:71
> +    int m_downloadLength;

Can this overflow?

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:79
> +    , m_downloadingToFile(downloadingToFile) { }

put the {} on different lines.

Would be nice to assert(!client); to make it clear that it should have a value.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:160
> +COMPILE_ASSERT((int)WebKit::DenyCrossOriginRequests == (int)WebCore::DenyCrossOriginRequests, enum_mismatch);

If these casts remain (in another version), use C++ style casts (static_cast).

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:176
> +    m_clientAdapter = adoptPtr(new ClientAdapter(this, m_client, request.downloadToFile()));

Have a static create method do this and make the constructor private.

> Source/WebKit/chromium/src/AssociatedURLLoader.h:35
>  #include "WebURLLoaderClient.h"

This include isn't needed anymore (only a fwd declaration).
Comment 16 David Levin 2011-03-01 15:17:04 PST
(In reply to comment #14)
> I guess my question was "Do you think it's important for the synchronous case to work now (or ever) and can it be added later, or should I connect this through WebCore as I did for downloadToFile stuff?"

I don't understand a few things:
1. Why there is any implementation if it doesn't work? Why not just crash (for example)?
2. Why do we have this method if it will do nothing? How do we know that no one will call it?

btw, there appear to be a lot of methods in the cpp file like AssociatedURLLoader::loadSynchronously which aren't declared in the .h file. I don't understand how that compiles.
Comment 17 Bill Budge 2011-03-01 17:02:54 PST
Created attachment 84328 [details]
Proposed Patch

Added static create method to ClientAdapter.
Made loadSynchronously method fail to indicate it's not supported.
Comment 18 Bill Budge 2011-03-01 17:05:15 PST
Just realized I didn't fully address your questions. The loadSynchronously method is part of the interface. I could implement it but it would require another round of changes to DocumentThreadableLoader and SubresourceLoader, and this method isn't currently needed in the pepper URL loader which is the client for this. That class doesn't support synchronous loading either.
Comment 19 David Levin 2011-03-01 17:51:05 PST
Comment on attachment 84328 [details]
Proposed Patch

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

r- due to not returning PassRefPtr from constructor (and the question about overflow).

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:58
> +    static ClientAdapter* create(WebURLLoader*, WebURLLoaderClient*, bool /*downloadToFile*/);

This should return a PassRefPtr<ClientAdapter>

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:80
> +    return new ClientAdapter(loader, client, downloadToFile);

Use the adoptPtr here (to convert to PassRefPtr).

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:114
> +    m_downloadLength += lengthReceived;

I still wonder about overflow. (Not likely but overflow is where security issues creep in.)

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:161
> +COMPILE_ASSERT_MATCHING_ENUM(UseAccessControl,  UseAccessControl);

two spaces after comma.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:162
> +COMPILE_ASSERT_MATCHING_ENUM(AllowCrossOriginRequests,  AllowCrossOriginRequests);

Ditto.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:186
> +    m_clientAdapter = adoptPtr(ClientAdapter::create(this, m_client, request.downloadToFile()));

adoptPtr shouldn't be here.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:200
> +       m_loader->setDefersLoading(defersLoading);

Should be indented one more space.
Comment 20 Bill Budge 2011-03-02 11:17:36 PST
Created attachment 84442 [details]
Proposed Patch

I think I can implement the loadSynchronously method in AssociatedURLLoader.cpp, without any other WebKit changes. Can that be another changelist?
Comment 21 WebKit Commit Bot 2011-03-03 02:29:02 PST
Comment on attachment 84442 [details]
Proposed Patch

Clearing flags on attachment: 84442

Committed r80219: <http://trac.webkit.org/changeset/80219>
Comment 22 WebKit Commit Bot 2011-03-03 02:29:09 PST
All reviewed patches have been landed.  Closing bug.
Comment 23 Mihai Parparita 2011-03-03 09:10:58 PST
Reopening since this was rolled out with http://trac.webkit.org/changeset/80244
Comment 24 Bill Budge 2011-03-04 18:39:07 PST
Created attachment 84840 [details]
Proposed Patch

Default AssociatedURLLoader should allow credentials so as not to break tests.
AssociatedURLLoader::cancel should release DocumentThreadableLoader.
Comment 25 WebKit Commit Bot 2011-03-07 00:46:36 PST
Comment on attachment 84840 [details]
Proposed Patch

Clearing flags on attachment: 84840

Committed r80458: <http://trac.webkit.org/changeset/80458>
Comment 26 WebKit Commit Bot 2011-03-07 00:46:44 PST
All reviewed patches have been landed.  Closing bug.
Comment 27 David Levin 2011-03-07 10:23:49 PST
This was rolled out again, see https://bugs.webkit.org/show_bug.cgi?id=53925
Comment 28 Mihai Parparita 2011-03-07 10:25:06 PST
(In reply to comment #27)
> This was rolled out again, see https://bugs.webkit.org/show_bug.cgi?id=53925

I think you meant https://bugs.webkit.org/show_bug.cgi?id=55863
Comment 29 Bill Budge 2011-03-10 10:20:20 PST
Created attachment 85345 [details]
Proposed Patch

The tests run identically to  the unpatched version on Mac. Will double check on Windows.
Comment 30 Bill Budge 2011-03-10 10:54:15 PST
Comment on attachment 85345 [details]
Proposed Patch

Hold off on reviewing. It still crashes on 1 test on Windows.
Comment 31 David Levin 2011-03-10 11:08:03 PST
(In reply to comment #29)
> Created an attachment (id=85345) [details]
> Proposed Patch
> 
> The tests run identically to  the unpatched version on Mac. Will double check on Windows.

Please indicate what changes were done between patches to make it easier to review. Thanks!
Comment 32 Bill Budge 2011-03-10 11:36:37 PST
This patch adds a clearClient method to the AssociatedURLLoader::ClientAdapter class. AssociatedURLLoader calls it to stop callbacks to the actual client after a load is canceled. This matches the previous implementations semantics.

I also fixed a typo that prevented the loadAsynchronously method from ever allowing cross origin requests.

As it stands, AssociatedURLLoader will allow cross origin requests if constructed with the original ctor. This is necessary for the following to pass:

/http/tests/security/local-video-source-from-remote.html

This makes another test pass which is supposed to fail:

/http/tests/security/xss-DENIED-xml-external-entity.xhtml

A future patch should make sure we can provide appropriately set up loaders to the different clients of WebFrame::CreateAssociatedLoader();
Comment 33 Bill Budge 2011-03-10 12:29:33 PST
Comment on attachment 85345 [details]
Proposed Patch

I reverted to the original and rebuilt DumpRenderTree, and it still crashes on this test, in exactly the same way. That leads me to think it's my local setup that is wrong. Do you think it's a bad idea to go ahead and see what the bots do?
Comment 34 Bill Budge 2011-03-11 11:18:01 PST
Tests pass on Windows. Please take a look.
Comment 35 Bill Budge 2011-03-11 11:26:24 PST
I want to make one minor change, which will help my next patch.
Comment 36 Bill Budge 2011-03-11 13:45:33 PST
Created attachment 85523 [details]
Proposed Patch

I changed AssociatedURLLoaderOptions struct to WebURLLoaderOptions, as this will need to be a parameter for WebFrame::createAssociatedURLLoader, and AssociatedURLLoader isn't part of the public WebKit interface.

I also made a minor semantic change, so the client will get didFinishLoading callbacks even if they cancel during the didDownloadData callback. This seems more correct, as at this point the load is finished and can't fail.
Comment 37 Bill Budge 2011-03-11 14:36:34 PST
Created attachment 85537 [details]
Proposed Patch

Fixed Copyright on WebURLLoader.h, and change WebURLLoaderCrossOriginRequestPolicy to WebCrossOriginRequestPolicy.
Comment 38 David Levin 2011-03-15 07:58:35 PDT
Comment on attachment 85537 [details]
Proposed Patch

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

> Source/WebKit/chromium/public/WebURLLoader.h:51
> +    WebURLLoaderOptions() : sniffContent(false), allowCredentials(false), forcePreflight(false), crossOriginRequestPolicy(DenyCrossOriginRequests) {}

Note we typically put a space in this construct "{ }".
Comment 39 WebKit Commit Bot 2011-03-15 10:24:47 PDT
Comment on attachment 85537 [details]
Proposed Patch

Clearing flags on attachment: 85537

Committed r81144: <http://trac.webkit.org/changeset/81144>
Comment 40 WebKit Commit Bot 2011-03-15 10:24:54 PDT
All reviewed patches have been landed.  Closing bug.
Comment 41 David Levin 2011-03-15 17:38:54 PDT
So close, but this appears to have broken the ui test NPAPITesterBase.GetURLRedirectNotification, which I saw here: http://build.chromium.org/p/chromium.webkit/builders/Vista%20Tests/

So it is best to roll it out one more time. I have a good feeling about next time!
Comment 42 David Levin 2011-03-15 17:50:25 PDT
Rolled out here: http://trac.webkit.org/changeset/81204
Comment 43 Bill Budge 2011-03-16 17:01:32 PDT
Created attachment 86002 [details]
Proposed Patch

I forgot to derive the ClientAdapter from the DocumentThreadableLoaderClient class that we went to so much trouble to make. DocumentThreadableLoader wasn't calling the willSendRequest method of the adapter as a result.
Comment 44 WebKit Commit Bot 2011-03-16 17:45:26 PDT
Comment on attachment 86002 [details]
Proposed Patch

Clearing flags on attachment: 86002

Committed r81303: <http://trac.webkit.org/changeset/81303>
Comment 45 WebKit Commit Bot 2011-03-16 17:45:34 PDT
All reviewed patches have been landed.  Closing bug.