[Android WebView] Only send extra headers for the main page

[Android WebView] Only send extra headers for the main page

Unlike Chromium, WebViewClassic has a very narrow set of conditions
when extra headers set for a navigation are actually added to requests.

To fix that, instead of passing the extra headers from AwContents.loadUrl
down to NavigationController, we stash them into our ResourceContext
and inject from AwResourceDispatcherHostDelegate when needed.

The resulting behavior seems to be compatible with WebViewClassic,
except that we also inject headers for back / forward navigations, as this
seems to be more consistent with the legacy behavior of injecting them
on page reload.

BUG=306873
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=239807

[sgurun-gerrit only, 2013-12-06 20:08:55]

https://codereview.chromium.org/99683006/diff/1/android_webview/browser/aw_re...
File android_webview/browser/aw_resource_context.cc (right):

https://codereview.chromium.org/99683006/diff/1/android_webview/browser/aw_re...
android_webview/browser/aw_resource_context.cc:46:
extra_headers_.erase(url.spec());
probably I missed it. Once extra headers are added for a URL, where do we get
rid of them later? do we always keep them?

https://codereview.chromium.org/99683006/diff/1/android_webview/browser/rende...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/99683006/diff/1/android_webview/browser/rende...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:448:
request->SetExtraRequestHeaderByName(it.name(), it.value(), true);
I was curious why you chose this rather than the simple SetExtraRequestHeaders..
Probably not to overwrite the existing ones, right? Maybe put a comment.

[mnaganov (inactive), 2013-12-07 16:10:26]

https://codereview.chromium.org/99683006/diff/1/android_webview/browser/aw_re...
File android_webview/browser/aw_resource_context.cc (right):

https://codereview.chromium.org/99683006/diff/1/android_webview/browser/aw_re...
android_webview/browser/aw_resource_context.cc:46:
extra_headers_.erase(url.spec());
On 2013/12/06 20:08:55, sgurun wrote:
> probably I missed it. Once extra headers are added for a URL, where do we get
> rid of them later? do we always keep them?

We get rid of them, if there will be another loadUrl call for the same URL but
no extra headers. We must keep the headers, because we need to inject them again
on reloads and back / forward navs. As we check the transition type, we will not
be injecting headers if the same page is loaded as a result of user-initiated
navigation (clicking a link) or as a child resource (e.g. as an iframe).

https://codereview.chromium.org/99683006/diff/1/android_webview/browser/rende...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/99683006/diff/1/android_webview/browser/rende...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:448:
request->SetExtraRequestHeaderByName(it.name(), it.value(), true);
On 2013/12/06 20:08:55, sgurun wrote:
> I was curious why you chose this rather than the simple
SetExtraRequestHeaders..
> Probably not to overwrite the existing ones, right? Maybe put a comment.

There is a difference between situations when SetExtraRequestHeaderByName and
SetExtraRequestHeaders can be called. SetExtraRequestHeaders may only be called
before request's Start() is called, while SetExtraRequestHeaderByName may also
be called before a new redirect in the request chain. Since I wanted to reuse
the code, I resorted to using the method that works in both cases. These
restrictions are reflected in the docs of the methods, do you think we need to
repeat them here?

[mkosiba (inactive), 2013-12-09 10:54:31]

Thanks Mikhail! Out of pure paranoia could I ask you to add a test that uses
window.location= and/or the History API too? For the history API you can try
copying some of the code from here:
https://codereview.chromium.org/68763012/diff/180001/android_webview/javatest...

https://codereview.chromium.org/99683006/diff/1/android_webview/browser/rende...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/99683006/diff/1/android_webview/browser/rende...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:433:
if (request_info->GetResourceType() != ResourceType::MAIN_FRAME) return;
uber-nit might fit in one line?:
    if (!request_info || request_info-> ...) return;

https://codereview.chromium.org/99683006/diff/1/android_webview/javatests/src...
File
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java
(right):

https://codereview.chromium.org/99683006/diff/1/android_webview/javatests/src...
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:103:
private void reloadSync(
nit: this could probably go to AwTestBase.

https://codereview.chromium.org/99683006/diff/1/android_webview/javatests/src...
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:248:
validateRequestHeaders(extraHeaders, webServer.getLastRequest(redirectedPath));
this looks like a potential race - loadUrlWithExtraHeadersSync waits for
onPageFinished once (which is the first load) but won't wait for the
onPageFinished for the redirect, so you might get occasional failures on this
line.

[mnaganov (inactive), 2013-12-09 13:58:01]

Thanks, Marcin!

I'm not sure that adding 'window.location' and 'window.history' tests will add
any value -- these facilities merely automate user actions of clicking links and
buttons, so they should be treated internally the same. Also, there are probably
million other ways to navigate via JS code, I don't want to test them all.

https://codereview.chromium.org/99683006/diff/1/android_webview/browser/rende...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/99683006/diff/1/android_webview/browser/rende...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:433:
if (request_info->GetResourceType() != ResourceType::MAIN_FRAME) return;
On 2013/12/09 10:54:31, mkosiba wrote:
> uber-nit might fit in one line?:
>     if (!request_info || request_info-> ...) return;

Unfortunately, this doesn't fit on one line. Only if you use a shorter name for
'request_info'.

https://codereview.chromium.org/99683006/diff/1/android_webview/javatests/src...
File
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java
(right):

https://codereview.chromium.org/99683006/diff/1/android_webview/javatests/src...
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:103:
private void reloadSync(
On 2013/12/09 10:54:31, mkosiba wrote:
> nit: this could probably go to AwTestBase.

Done.

https://codereview.chromium.org/99683006/diff/1/android_webview/javatests/src...
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:248:
validateRequestHeaders(extraHeaders, webServer.getLastRequest(redirectedPath));
On 2013/12/09 10:54:31, mkosiba wrote:
> this looks like a potential race - loadUrlWithExtraHeadersSync waits for
> onPageFinished once (which is the first load) but won't wait for the
> onPageFinished for the redirect, so you might get occasional failures on this
> line.

In fact, we don't call onPageFinished for redirect intermediates, so it will
only be called after the target page has been loaded.

[mnaganov (inactive), 2013-12-09 14:01:36]

On 2013/12/09 13:58:01, Mikhail Naganov (Chromium) wrote:
> ... they should be treated internally the same.

By this I mean, "the same as if this action is done by the Content layer
embedded via API calls".

[mkosiba (inactive), 2013-12-09 16:35:56]

Thanks Mikhail!

The only reason I listed window.location/history was because I think they result
in a different transition type than link clicks. I don't feel strongly about
adding them in.

https://codereview.chromium.org/99683006/diff/1/android_webview/javatests/src...
File
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java
(right):

https://codereview.chromium.org/99683006/diff/1/android_webview/javatests/src...
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:248:
validateRequestHeaders(extraHeaders, webServer.getLastRequest(redirectedPath));
On 2013/12/09 13:58:01, Mikhail Naganov (Chromium) wrote:
> On 2013/12/09 10:54:31, mkosiba wrote:
> > this looks like a potential race - loadUrlWithExtraHeadersSync waits for
> > onPageFinished once (which is the first load) but won't wait for the
> > onPageFinished for the redirect, so you might get occasional failures on
this
> > line.
> 
> In fact, we don't call onPageFinished for redirect intermediates, so it will
> only be called after the target page has been loaded.

ah, good to know :)

[mnaganov (inactive), 2013-12-09 17:01:17]

On 2013/12/09 16:35:56, mkosiba wrote:
> Thanks Mikhail!
> 
> The only reason I listed window.location/history was because I think they
result
> in a different transition type than link clicks. I don't feel strongly about
> adding them in.
>

Their transition types are no different from the ones we are already testing,
I've checked that.
Setting window.location.href emits PAGE_TRANSITION_LINK, and history transitions
are covered with PAGE_TRANSITION_FORWARD_BACK.
 
>
https://codereview.chromium.org/99683006/diff/1/android_webview/javatests/src...
> File
>
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java
> (right):
> 
>
https://codereview.chromium.org/99683006/diff/1/android_webview/javatests/src...
>
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:248:
> validateRequestHeaders(extraHeaders,
webServer.getLastRequest(redirectedPath));
> On 2013/12/09 13:58:01, Mikhail Naganov (Chromium) wrote:
> > On 2013/12/09 10:54:31, mkosiba wrote:
> > > this looks like a potential race - loadUrlWithExtraHeadersSync waits for
> > > onPageFinished once (which is the first load) but won't wait for the
> > > onPageFinished for the redirect, so you might get occasional failures on
> this
> > > line.
> > 
> > In fact, we don't call onPageFinished for redirect intermediates, so it will
> > only be called after the target page has been loaded.
> 
> ah, good to know :)

[mkosiba (inactive), 2013-12-09 17:04:16]

On 2013/12/09 17:01:17, Mikhail Naganov (Chromium) wrote:
> On 2013/12/09 16:35:56, mkosiba wrote:
> > Thanks Mikhail!
> > 
> > The only reason I listed window.location/history was because I think they
> result
> > in a different transition type than link clicks. I don't feel strongly about
> > adding them in.
> >
> 
> Their transition types are no different from the ones we are already testing,
> I've checked that.
> Setting window.location.href emits PAGE_TRANSITION_LINK, and history
transitions
> are covered with PAGE_TRANSITION_FORWARD_BACK.

Thanks for following up. No further comments from me then. LGTM

[benm (inactive), 2013-12-09 17:49:11]

thanks Mikhail!

https://codereview.chromium.org/99683006/diff/20001/android_webview/browser/r...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/99683006/diff/20001/android_webview/browser/r...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:448:
request->SetExtraRequestHeaderByName(it.name(), it.value(), true);
What's the behaviour in Classic vs. Chromium WebView if we try to overwrite a
header that's already set, and is it consistent? IIRC Classic would overwrite
with the headers provided by loadUrl.

https://codereview.chromium.org/99683006/diff/20001/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/99683006/diff/20001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:916: if
(mNativeAwContents != 0) nativeSetExtraHeadersForUrl(
split this into separate lines please.

https://codereview.chromium.org/99683006/diff/20001/android_webview/javatests...
File
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java
(right):

https://codereview.chromium.org/99683006/diff/20001/android_webview/javatests...
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:158:
"x-extraHeaders2", "EXTRA-HEADER-DATA2"
Maybe add a test case for overwriting a header that's normally already set, like
User-Agent.

https://codereview.chromium.org/99683006/diff/20001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java
(right):

https://codereview.chromium.org/99683006/diff/20001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:239:
if (headerBuilder.length() > 0) headerBuilder.append("\r\n");
should this be |delimiter|?

[sgurun-gerrit only, 2013-12-09 18:58:27]

lgtm

https://codereview.chromium.org/99683006/diff/1/android_webview/browser/rende...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/99683006/diff/1/android_webview/browser/rende...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:448:
request->SetExtraRequestHeaderByName(it.name(), it.value(), true);
I see. missed it. I would prefer a comment (since I already missed it) but
either way is fine.

On 2013/12/07 16:10:26, Mikhail Naganov (Chromium) wrote:
> On 2013/12/06 20:08:55, sgurun wrote:
> > I was curious why you chose this rather than the simple
> SetExtraRequestHeaders..
> > Probably not to overwrite the existing ones, right? Maybe put a comment.
> 
> There is a difference between situations when SetExtraRequestHeaderByName and
> SetExtraRequestHeaders can be called. SetExtraRequestHeaders may only be
called
> before request's Start() is called, while SetExtraRequestHeaderByName may also
> be called before a new redirect in the request chain. Since I wanted to reuse
> the code, I resorted to using the method that works in both cases. These
> restrictions are reflected in the docs of the methods, do you think we need to
> repeat them here?

[mnaganov (inactive), 2013-12-10 11:03:30]

Thanks, Ben! An astonishingly useful review -- catched real bugs.

https://codereview.chromium.org/99683006/diff/1/android_webview/browser/rende...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/99683006/diff/1/android_webview/browser/rende...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:448:
request->SetExtraRequestHeaderByName(it.name(), it.value(), true);
On 2013/12/09 18:58:27, sgurun wrote:
> I see. missed it. I would prefer a comment (since I already missed it) but
> either way is fine.
> 
> On 2013/12/07 16:10:26, Mikhail Naganov (Chromium) wrote:
> > On 2013/12/06 20:08:55, sgurun wrote:
> > > I was curious why you chose this rather than the simple
> > SetExtraRequestHeaders..
> > > Probably not to overwrite the existing ones, right? Maybe put a comment.
> > 
> > There is a difference between situations when SetExtraRequestHeaderByName
and
> > SetExtraRequestHeaders can be called. SetExtraRequestHeaders may only be
> called
> > before request's Start() is called, while SetExtraRequestHeaderByName may
also
> > be called before a new redirect in the request chain. Since I wanted to
reuse
> > the code, I resorted to using the method that works in both cases. These
> > restrictions are reflected in the docs of the methods, do you think we need
to
> > repeat them here?
> 

Now, thanks to Ben's comment, I have changed the behavior to not override
existing headers, which rules out using of SetExtraRequestHeaders, so no comment
is needed.

https://codereview.chromium.org/99683006/diff/20001/android_webview/browser/r...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/99683006/diff/20001/android_webview/browser/r...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:448:
request->SetExtraRequestHeaderByName(it.name(), it.value(), true);
On 2013/12/09 17:49:11, benm wrote:
> What's the behaviour in Classic vs. Chromium WebView if we try to overwrite a
> header that's already set, and is it consistent? IIRC Classic would overwrite
> with the headers provided by loadUrl.

That's a good catch! In fact, the old WebView seems to never override existing
headers, so using 'true' here is incorrect. Changed to 'false'.

https://codereview.chromium.org/99683006/diff/20001/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/99683006/diff/20001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:916: if
(mNativeAwContents != 0) nativeSetExtraHeadersForUrl(
On 2013/12/09 17:49:11, benm wrote:
> split this into separate lines please.

Added brackets. I'm wondering, how did I ever wrote this :)

https://codereview.chromium.org/99683006/diff/20001/android_webview/javatests...
File
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java
(right):

https://codereview.chromium.org/99683006/diff/20001/android_webview/javatests...
android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java:158:
"x-extraHeaders2", "EXTRA-HEADER-DATA2"
On 2013/12/09 17:49:11, benm wrote:
> Maybe add a test case for overwriting a header that's normally already set,
like
> User-Agent.

Done.

https://codereview.chromium.org/99683006/diff/20001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java
(right):

https://codereview.chromium.org/99683006/diff/20001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:239:
if (headerBuilder.length() > 0) headerBuilder.append("\r\n");
On 2013/12/09 17:49:11, benm wrote:
> should this be |delimiter|?

Sure, great catch! Fixed.

[mnaganov (inactive), 2013-12-10 11:05:38]

bulach@: Can you please take a look at the content/public/android/ change?

[benm (inactive), 2013-12-10 14:27:45]

lgtm

https://codereview.chromium.org/99683006/diff/40001/android_webview/browser/a...
File android_webview/browser/aw_browser_context.h (right):

https://codereview.chromium.org/99683006/diff/40001/android_webview/browser/a...
android_webview/browser/aw_browser_context.h:32: class URLRequestContextGetter;
nit: needed?

[mnaganov (inactive), 2013-12-10 15:16:55]

Thanks, Ben!

Marcus?

https://codereview.chromium.org/99683006/diff/40001/android_webview/browser/a...
File android_webview/browser/aw_browser_context.h (right):

https://codereview.chromium.org/99683006/diff/40001/android_webview/browser/a...
android_webview/browser/aw_browser_context.h:32: class URLRequestContextGetter;
On 2013/12/10 14:27:46, benm wrote:
> nit: needed?

Nope. Removed.

[bulach, 2013-12-10 15:16:57]

lgtm, just some nits.. thanks!

https://codereview.chromium.org/99683006/diff/40001/android_webview/browser/a...
File android_webview/browser/aw_resource_context.h (right):

https://codereview.chromium.org/99683006/diff/40001/android_webview/browser/a...
android_webview/browser/aw_resource_context.h:31: void SetExtraHeaders(const
GURL& url, std::string& headers);
nit: const for headers?

also, I think non-virtuals should be listed before the virtuals above.

https://codereview.chromium.org/99683006/diff/40001/android_webview/browser/a...
android_webview/browser/aw_resource_context.h:36: base::Lock lock_;
nit: it'd be nice to say what this is protecting..

https://codereview.chromium.org/99683006/diff/40001/android_webview/browser/r...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/99683006/diff/40001/android_webview/browser/r...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:421:
request_info->GetPageTransition(), content::PAGE_TRANSITION_RELOAD);
nit: perhaps unroll request_info->GetPageTransition()? it's called three times
here..

https://codereview.chromium.org/99683006/diff/40001/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/99683006/diff/40001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:890: if
(mNativeAwContents != 0) {
how about testing for params.getExtraHttpRequestHeadersString() being non-empty
and avoid calling setExtraHeaders all the time?

[mnaganov (inactive), 2013-12-10 16:00:36]

Thanks, Marcus!

Thanks again, everyone! Your suggestions helped to make tremendous improvements
of the patch.

https://codereview.chromium.org/99683006/diff/40001/android_webview/browser/a...
File android_webview/browser/aw_resource_context.h (right):

https://codereview.chromium.org/99683006/diff/40001/android_webview/browser/a...
android_webview/browser/aw_resource_context.h:31: void SetExtraHeaders(const
GURL& url, std::string& headers);
On 2013/12/10 15:16:57, bulach wrote:
> nit: const for headers?
> 
> also, I think non-virtuals should be listed before the virtuals above.

Changed to const.

There is no explicit style guide on placing non-virtuals vs. virtuals. But as
AwBrowserContext puts non-virtuals first, made the same here for consistency.

https://codereview.chromium.org/99683006/diff/40001/android_webview/browser/a...
android_webview/browser/aw_resource_context.h:36: base::Lock lock_;
On 2013/12/10 15:16:57, bulach wrote:
> nit: it'd be nice to say what this is protecting..

Renamed to extra_headers_lock_.

https://codereview.chromium.org/99683006/diff/40001/android_webview/browser/r...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/99683006/diff/40001/android_webview/browser/r...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:421:
request_info->GetPageTransition(), content::PAGE_TRANSITION_RELOAD);
On 2013/12/10 15:16:57, bulach wrote:
> nit: perhaps unroll request_info->GetPageTransition()? it's called three times
> here..

Done.

https://codereview.chromium.org/99683006/diff/40001/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/99683006/diff/40001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:890: if
(mNativeAwContents != 0) {
On 2013/12/10 15:16:57, bulach wrote:
> how about testing for params.getExtraHttpRequestHeadersString() being
non-empty
> and avoid calling setExtraHeaders all the time?

No, we need to call setExtraHeaders every time, since we also need to be able to
reset previously set headers. That said, we could avoid calling setExtraHeaders,
if the state of the headers is unchanged, but then we will need to look at their
current state first, which will cost an extra JNI call.

[commit-bot: I haz the power, 2013-12-10 16:01:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnaganov@chromium.org/99683006/80001

[commit-bot: I haz the power, 2013-12-10 18:43:26]

Change committed as 239807