Bug 90893 – In FrameLoader, don't apply user agent if one is already set.

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Page Loading

[Viet-Trung Luu]

This would allow things that require custom user agents (e.g., some Pepper plugins -- see https://chromiumcodereview.appspot.com/10701119 ) to set them.

[Viet-Trung Luu]

Created attachment 151482 [details]
Pretty much the minimal patch that allows me to do what I need....

[Adam Barth]

Comment on attachment 151482 [details]
Pretty much the minimal patch that allows me to do what I need....

This patch requires a test.  You might want to write a Chromium unit test that calls the WebKit API in a similar manner to how Pepper does.

[Adam Barth]

http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/tests has a testing framework that might be useful.

[Viet-Trung Luu]

Created attachment 151519 [details]
Added test. Now dependent on https://chromiumcodereview.appspot.com/10735037 .

[Adam Barth]

Comment on attachment 151519 [details]
Added test. Now dependent on https://chromiumcodereview.appspot.com/10735037 .

LGTM.  Thanks for the test.

[Viet-Trung Luu]

Created attachment 151528 [details]
Updated ChangeLog and rolled DEPS.

[WebKit Review Bot]

Attachment 151528 [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
Source/WebKit/chromium/tests/AssociatedURLLoaderTest.cpp:621:  user_agent_header_name is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit/chromium/tests/AssociatedURLLoaderTest.cpp:641:  sent_request is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit/chromium/tests/AssociatedURLLoaderTest.cpp:642:  default_user_agent_value is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit/chromium/tests/AssociatedURLLoaderTest.cpp:650:  user_agent_value is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit/chromium/tests/AssociatedURLLoaderTest.cpp:663:  sent_user_agent_value is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 5 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 151528 [details]
Updated ChangeLog and rolled DEPS.

Please fix these style issues before landing.  WebKit uses camelCase for variable names.

[Viet-Trung Luu]

Created attachment 151534 [details]
variableNameStyleFixes.

[WebKit Review Bot]

Comment on attachment 151534 [details]
variableNameStyleFixes.

Attachment 151534 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13208148

New failing tests:
http/tests/inspector/extensions-useragent.html

[WebKit Review Bot]

Created attachment 151559 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[WebKit Review Bot]

Comment on attachment 151528 [details]
Updated ChangeLog and rolled DEPS.

Attachment 151528 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13166603

New failing tests:
http/tests/inspector/extensions-useragent.html

[WebKit Review Bot]

Created attachment 151560 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[WebKit Review Bot]

Comment on attachment 151534 [details]
variableNameStyleFixes.

Attachment 151534 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13198163

New failing tests:
http/tests/inspector/extensions-useragent.html

[WebKit Review Bot]

Created attachment 151566 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[WebKit Review Bot]

Comment on attachment 151528 [details]
Updated ChangeLog and rolled DEPS.

Attachment 151528 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13209166

New failing tests:
http/tests/inspector/extensions-useragent.html

[WebKit Review Bot]

Created attachment 151567 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Alexey Proskuryakov]

Comment on attachment 151534 [details]
variableNameStyleFixes.

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

> Source/WebCore/loader/FrameLoader.cpp:2870
>  void FrameLoader::applyUserAgent(ResourceRequest& request)
>  {
> +    if (!request.httpUserAgent().isNull())
> +        return;

This function no longer just applies user agent. You should rename it to not be misleading.

[Viet-Trung Luu]

Ugh. The layout test failure revealed a fatal flaw with my plan: my change resulted in InspectorInstrumentation::applyUserAgentOverride() (done in FrameLoader::userAgent()) not getting a chance for reloads.

Arguably, in the long term, a more coherent way of applying user agents to requests would be desirable. But, in the short term, I'll take another approach (new patch coming up shortly).

[Viet-Trung Luu]

Created attachment 152090 [details]
New approach: add custom user agent to ResourceRequest, to be handled by the FrameLoaderClientImpl.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Maybe the embedder should do this itself using ExtraData?

[Viet-Trung Luu]

(In reply to comment #22)
> Maybe the embedder should do this itself using ExtraData?

Assuming you mean:

- Have the owner/user/caller/whatever of the AssociatedURLLoader set an appropriate WebURLRequest::ExtraData.
- Then have the AssociatedURLLoader's WebURLLoaderClient use that to set the user agent (in willSendRequest()).

I agree that that'd be the most natural thing. Unfortunately, it has a fatal flaw (the reason for which I don't fully understand, but Nate knows more about this[*]): the AssociatedURLLoader::ClientAdapter::willSendRequest() (which would call its clients willSendRequest()) doesn't get called for the initial request -- only for redirects.

At least I *think* that's the case (i.e., I think I tried this), but let me double-check (since the code doesn't make this obvious)....


[*] I believe he said that the AssociatedURLLoader doesn't get set up as a client in time. Why, I don't know....

[Adam Barth]

Interesting.  That's probably the core of this bug (although it might be tough to fix it there).

[WebKit Review Bot]

Comment on attachment 152090 [details]
New approach: add custom user agent to ResourceRequest, to be handled by the FrameLoaderClientImpl.

Attachment 152090 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13201870

[Viet-Trung Luu]

(In reply to comment #23)
> At least I *think* that's the case (i.e., I think I tried this), but let me double-check (since the code doesn't make this obvious)....

Definitely the case that AssociatedURLLoader::ClientAdapter::willSendRequest() only gets called for redirects, and not the original request. :(

[Viet-Trung Luu]

Created attachment 152120 [details]
Updated to ToT.

[Nate Chapin]

(In reply to comment #23)
> (In reply to comment #22)
> > Maybe the embedder should do this itself using ExtraData?
> 
> Assuming you mean:
> 
> - Have the owner/user/caller/whatever of the AssociatedURLLoader set an appropriate WebURLRequest::ExtraData.
> - Then have the AssociatedURLLoader's WebURLLoaderClient use that to set the user agent (in willSendRequest()).
> 
> I agree that that'd be the most natural thing. Unfortunately, it has a fatal flaw (the reason for which I don't fully understand, but Nate knows more about this[*]): the AssociatedURLLoader::ClientAdapter::willSendRequest() (which would call its clients willSendRequest()) doesn't get called for the initial request -- only for redirects.
> 
> At least I *think* that's the case (i.e., I think I tried this), but let me double-check (since the code doesn't make this obvious)....
> 
> 
> [*] I believe he said that the AssociatedURLLoader doesn't get set up as a client in time. Why, I don't know....

The reason is the timing of starting a request for a CachedResource. Basically, when DocumentThreadableLoader calls CachedResourceLoader::requestRawResource(), the decision on whether to start a resource load is made synchronously, and the load is actually started synchronously. The CachedResourceClient, therefore, doesn't have the opportunity to call CachedResource::addClient() to add itself until after the request has started and willSendRequest() has been called.

I guess we could change this, and it shouldn't be too hard to re-plumb, but I feel like it might make the code a little less readable. The main downside is that there would need to be a CachedResourceClient* parameter on a whole bunch of functions in CachedResourceLoader, but since only CachedRawResource exposes anything analogous to willSendrequest(), it would only be used in a small subset of cases.

[Adam Barth]

That makes sense.  My main worry is that we'll want to do other things during will send request, but I guess we can solve that problem when it comes up.

[Adam Barth]

Comment on attachment 152120 [details]
Updated to ToT.

This patch is fine with me.  My only question is whether we're applying the custom user agent at the right layer.  It's slightly odd for the state to be in WebCore but to use the state at the WebKit layer.

I'd like to defer to Nate for the final review, if that's ok.

[Darin Fisher (:fishd, Google)]

Comment on attachment 152120 [details]
Updated to ToT.

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

> Source/Platform/chromium/public/WebURLRequest.h:176
> +    WEBKIT_EXPORT void setCustomUserAgent(const WebString&);

question:

which takes precedence?  WebFrameClient::userAgentOverride or WebURLRequest::setCustomUserAgent?

which should take precedence?

[Viet-Trung Luu]

(In reply to comment #31)
> (From update of attachment 152120 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152120&action=review
> 
> > Source/Platform/chromium/public/WebURLRequest.h:176
> > +    WEBKIT_EXPORT void setCustomUserAgent(const WebString&);
> 
> question:
> 
> which takes precedence?  WebFrameClient::userAgentOverride or WebURLRequest::setCustomUserAgent?
> 
> which should take precedence?

WebURLRequest::setCustomUserAgent(), since it's more specific (applying to a request, as opposed to a URL).

[Darin Fisher (:fishd, Google)]

Suppose someone wants to force all URLs to load with a given User-Agent.  Do we need to have some other mechanism to implement that?

[Darin Fisher (:fishd, Google)]

(In reply to comment #33)
> Suppose someone wants to force all URLs to load with a given User-Agent.  Do we need to have some other mechanism to implement that?

Anyways, we should at least just document that setCustomUserAgent overrides both WebKit::Platform::userAgent() as well as WebKit::WebFrameClient::userAgentOverride().

What about the WebInspector's override UA capability?  I think setCustomUserAgent overrides that too.  (see InspectorInstrumentation::applyUserAgentOverride.)  Is that desirable too?

[Darin Fisher (:fishd, Google)]

By the way, it occurs to me that you could also implement this patch entirely on the Chromium side.

WebURLRequest has an extraData field that could be extended with a CustomUserAgent field.  The implementation of willSendRequest in render_view_impl.cc could read that field and apply it to the WebURLRequest.

This might be slightly cleaner since the embedder (render_view_impl.cc) implements both userAgentOverride as well as willSendRequest, so if it wants to make code in willSendRequest override code in userAgentOverride, then OK.

The advantage is that we avoid having to make WebKit understand N different ways to override the UA string.

[Viet-Trung Luu]

(In reply to comment #35)
> By the way, it occurs to me that you could also implement this patch entirely on the Chromium side.
> 
> WebURLRequest has an extraData field that could be extended with a CustomUserAgent field.  The implementation of willSendRequest in render_view_impl.cc could read that field and apply it to the WebURLRequest.
> 
> This might be slightly cleaner since the embedder (render_view_impl.cc) implements both userAgentOverride as well as willSendRequest, so if it wants to make code in willSendRequest override code in userAgentOverride, then OK.
> 
> The advantage is that we avoid having to make WebKit understand N different ways to override the UA string.

This is what I did (https://chromiumcodereview.appspot.com/10780019/, landed Chromium r147060).