Store and use last base URL between DidStart / DidStopLoading

Store and use last base URL between DidStart / DidStopLoading

As it turns out that the committed entry may omit the base
URL if there has been a javascript: URL navigation after
LoadDataWithBaseURL. Thus we have to store the base url on
DidStartLoading and use it in DidStopLoading / DidFinishLoad.

We have to preserve the old hack with SetToBaseURLForDataURLIfNeeded
because if the base URL is not valid, DidFinishLoad doesn't
receive it from the renderer.

This change is a short-term workaround. The correct solution
is to pass a flag that the load was through LoadDataWithBaseURL
via Blink, so base URL can be restored correctly within
NavigationController.

BUG=594001
Committed: https://crrev.com/ce1f4d009316d8f5f4c49b18e1bd94687d80c59d
Cr-Commit-Position: refs/heads/master@{#381108}

[mnaganov (inactive), 2016-03-11 01:58:43]

Description was changed from

==========
Fix new NavigationEntry creation for loadDataWithBaseURL

Usually a NavigationEntry is cloned from the pending entry,
but in case when the pending entry isn't used, base URL must
be set from DidCommitProvisionalLoad message params.

BUG=594001
==========

to

==========
Fix new NavigationEntry creation for loadDataWithBaseURL

Usually a NavigationEntry is cloned from the pending entry,
but in case when the pending entry isn't used, base URL must
be set from DidCommitProvisionalLoad message params.

BUG=594001
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[mnaganov (inactive), 2016-03-11 02:01:06]

mnaganov@chromium.org changed reviewers:
+ creis@chromium.org

[mnaganov (inactive), 2016-03-11 02:01:07]

Hi Charlie, PTAL! clamy@ is OOO.

[Charlie Reis, 2016-03-11 23:32:09]

https://codereview.chromium.org/1779363004/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1779363004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl.cc:1148:
new_entry->SetBaseURLForDataURL(params.base_url);
This seems too broad.  We only use SetBaseURLForDataURL when using
loadDataFromBaseURL, not for all data: URLs.  We don't want to call
SetBaseURLForDataURL in normal data: URL navigations because there's other logic
that checks GetBaseURLForDataURL to distinguish between loadDataFromBaseURL and
normal data: URL navigations.

Can we narrow this to only apply in the loadDataFromBaseURL case?

[mnaganov (inactive), 2016-03-12 01:18:14]

Description was changed from

==========
Fix new NavigationEntry creation for loadDataWithBaseURL

Usually a NavigationEntry is cloned from the pending entry,
but in case when the pending entry isn't used, base URL must
be set from DidCommitProvisionalLoad message params.

BUG=594001
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Store and use last base URL between DidStart / DidStopLoading

As it turns out that the committed entry may omit the base
URL if there has been a javascript: URL navigation in between,
it's more reliable to store the base url on DidStartLoading
and use it in DidStopLoading.

As DidFinishLoad receives a correct base URL from the renderer,
there is no need anymore to update it, and thus the old hack
with SetToBaseURLForDataURLIfNeeded can be removed.

BUG=594001
==========

[mnaganov (inactive), 2016-03-12 01:18:14]

mnaganov@chromium.org changed reviewers:
+ boliu@chromium.org
- creis@chromium.org

[mnaganov (inactive), 2016-03-14 18:13:22]

Description was changed from

==========
Store and use last base URL between DidStart / DidStopLoading

As it turns out that the committed entry may omit the base
URL if there has been a javascript: URL navigation in between,
it's more reliable to store the base url on DidStartLoading
and use it in DidStopLoading.

As DidFinishLoad receives a correct base URL from the renderer,
there is no need anymore to update it, and thus the old hack
with SetToBaseURLForDataURLIfNeeded can be removed.

BUG=594001
==========

to

==========
Store and use last base URL between DidStart / DidStopLoading

As it turns out that the committed entry may omit the base
URL if there has been a javascript: URL navigation after
LoadDataWithBaseURL. Thus we have to store the base url on
DidStartLoading and use it in DidStopLoading / DidFinishLoad.

We have to preserve the old hack with SetToBaseURLForDataURLIfNeeded
because if the base URL is not valid, DidFinishLoad doesn't
receive it from the renderer.

This change is a short-term workaround. The correct solution
is to pass a flag that the load was through LoadDataWithBaseURL
via Blink, so base URL can be restored correctly within
NavigationController.

BUG=594001
==========

[mnaganov (inactive), 2016-03-14 18:14:23]

Bo, I have managed to provide a workaround on the Android level, without
touching NavigationController, so changing the reviewer to you.

[boliu, 2016-03-14 18:34:04]

I don't own the content code btw :)

https://codereview.chromium.org/1779363004/diff/40001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/LoadDataWithBaseUrlTest.java
(right):

https://codereview.chromium.org/1779363004/diff/40001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/LoadDataWithBaseUrlTest.java:377:
// See b/275348709.
Use crbug in source code. Also should put this into the body of the test.
Comments referring to bugs are are generally referring to flaky tests..

https://codereview.chromium.org/1779363004/diff/40001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/LoadDataWithBaseUrlTest.java:397:
final String fixedBaseUrl = "http:";
The fixed url is still not a valid url?

(I thought the fix url should be an empty data url in this case..?)

https://codereview.chromium.org/1779363004/diff/40001/content/browser/android...
File content/browser/android/web_contents_observer_proxy.cc (right):

https://codereview.chromium.org/1779363004/diff/40001/content/browser/android...
content/browser/android/web_contents_observer_proxy.cc:96: if (auto entry =
web_contents()->GetController().GetPendingEntry()) {
auto* ? Actually style guide doesn't really say anything about that..

https://codereview.chromium.org/1779363004/diff/40001/content/browser/android...
content/browser/android/web_contents_observer_proxy.cc:228:
SetToBaseURLForDataURLIfNeeded(&url_string);
so iiuc DidFinishLoad is actually fired *between* DidStartLoading and
DidStopLoading? That should live in a comment somewhere imo

[mnaganov (inactive), 2016-03-14 20:10:57]

I know you are not an owner, but you were definitely dealing with
loadDataWithBaseURL before :)

https://codereview.chromium.org/1779363004/diff/40001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/LoadDataWithBaseUrlTest.java
(right):

https://codereview.chromium.org/1779363004/diff/40001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/LoadDataWithBaseUrlTest.java:377:
// See b/275348709.
On 2016/03/14 18:34:04, boliu wrote:
> Use crbug in source code. Also should put this into the body of the test.
> Comments referring to bugs are are generally referring to flaky tests..

Done.

https://codereview.chromium.org/1779363004/diff/40001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/LoadDataWithBaseUrlTest.java:397:
final String fixedBaseUrl = "http:";
On 2016/03/14 18:34:04, boliu wrote:
> The fixed url is still not a valid url?
> 
> (I thought the fix url should be an empty data url in this case..?)

This is what happens in the case when there is no "javascript:" loadUrl, because
in the observer we take a `possibly_invalid_spec` from the last committed
entry's base url.

testInvalidBaseUrl code says that the base url returned in this case is
"undefined". Since this change is about fixing the behavior when "javascript:"
is loaded, I wouldn't like changing any other behavior. I added a comment to the
observer code to maybe reconsider this later.

https://codereview.chromium.org/1779363004/diff/40001/content/browser/android...
File content/browser/android/web_contents_observer_proxy.cc (right):

https://codereview.chromium.org/1779363004/diff/40001/content/browser/android...
content/browser/android/web_contents_observer_proxy.cc:96: if (auto entry =
web_contents()->GetController().GetPendingEntry()) {
On 2016/03/14 18:34:04, boliu wrote:
> auto* ? Actually style guide doesn't really say anything about that..

I don't think it makes any difference here.

https://codereview.chromium.org/1779363004/diff/40001/content/browser/android...
content/browser/android/web_contents_observer_proxy.cc:228:
SetToBaseURLForDataURLIfNeeded(&url_string);
On 2016/03/14 18:34:04, boliu wrote:
> so iiuc DidFinishLoad is actually fired *between* DidStartLoading and
> DidStopLoading? That should live in a comment somewhere imo

For a long time actually. See the description of
https://codereview.chromium.org/1432083004. This is documented by the code of
AwWebContentsObserver.java.

[boliu, 2016-03-14 20:24:53]

lgtm from my perspective

https://codereview.chromium.org/1779363004/diff/40001/content/browser/android...
File content/browser/android/web_contents_observer_proxy.cc (right):

https://codereview.chromium.org/1779363004/diff/40001/content/browser/android...
content/browser/android/web_contents_observer_proxy.cc:228:
SetToBaseURLForDataURLIfNeeded(&url_string);
On 2016/03/14 20:10:57, mnaganov wrote:
> On 2016/03/14 18:34:04, boliu wrote:
> > so iiuc DidFinishLoad is actually fired *between* DidStartLoading and
> > DidStopLoading? That should live in a comment somewhere imo
> 
> For a long time actually. See the description of
> https://codereview.chromium.org/1432083004. This is documented by the code of
> AwWebContentsObserver.java. 

I mean that comment should live around where base_url_of_last_started_data_url_,
or maybe in SetToBaseURLForDataURLIfNeeded. Not totally obvious that this
affects DidFinishLoad as well.

[mnaganov (inactive), 2016-03-14 20:28:51]

mnaganov@chromium.org changed reviewers:
+ tedchoc@chromium.org

[mnaganov (inactive), 2016-03-14 20:28:51]

Thanks, Bo!

Ted, can you please take a look?

https://codereview.chromium.org/1779363004/diff/40001/content/browser/android...
File content/browser/android/web_contents_observer_proxy.cc (right):

https://codereview.chromium.org/1779363004/diff/40001/content/browser/android...
content/browser/android/web_contents_observer_proxy.cc:228:
SetToBaseURLForDataURLIfNeeded(&url_string);
On 2016/03/14 20:24:53, boliu wrote:
> On 2016/03/14 20:10:57, mnaganov wrote:
> > On 2016/03/14 18:34:04, boliu wrote:
> > > so iiuc DidFinishLoad is actually fired *between* DidStartLoading and
> > > DidStopLoading? That should live in a comment somewhere imo
> > 
> > For a long time actually. See the description of
> > https://codereview.chromium.org/1432083004. This is documented by the code
of
> > AwWebContentsObserver.java. 
> 
> I mean that comment should live around where
base_url_of_last_started_data_url_,
> or maybe in SetToBaseURLForDataURLIfNeeded. Not totally obvious that this
> affects DidFinishLoad as well.

Done.

[Ted C, 2016-03-14 20:49:32]

lgtm

[mnaganov (inactive), 2016-03-14 22:15:23]

The CQ bit was checked by mnaganov@chromium.org

[mnaganov (inactive), 2016-03-14 22:15:24]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/1779363004/#ps80001
(title: "Added comment")

[commit-bot: I haz the power, 2016-03-14 22:15:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1779363004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1779363004/80001

[commit-bot: I haz the power, 2016-03-14 23:25:02]

Description was changed from

==========
Store and use last base URL between DidStart / DidStopLoading

As it turns out that the committed entry may omit the base
URL if there has been a javascript: URL navigation after
LoadDataWithBaseURL. Thus we have to store the base url on
DidStartLoading and use it in DidStopLoading / DidFinishLoad.

We have to preserve the old hack with SetToBaseURLForDataURLIfNeeded
because if the base URL is not valid, DidFinishLoad doesn't
receive it from the renderer.

This change is a short-term workaround. The correct solution
is to pass a flag that the load was through LoadDataWithBaseURL
via Blink, so base URL can be restored correctly within
NavigationController.

BUG=594001
==========

to

==========
Store and use last base URL between DidStart / DidStopLoading

As it turns out that the committed entry may omit the base
URL if there has been a javascript: URL navigation after
LoadDataWithBaseURL. Thus we have to store the base url on
DidStartLoading and use it in DidStopLoading / DidFinishLoad.

We have to preserve the old hack with SetToBaseURLForDataURLIfNeeded
because if the base URL is not valid, DidFinishLoad doesn't
receive it from the renderer.

This change is a short-term workaround. The correct solution
is to pass a flag that the load was through LoadDataWithBaseURL
via Blink, so base URL can be restored correctly within
NavigationController.

BUG=594001
==========

[commit-bot: I haz the power, 2016-03-14 23:25:03]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-14 23:27:05]

Description was changed from

==========
Store and use last base URL between DidStart / DidStopLoading

As it turns out that the committed entry may omit the base
URL if there has been a javascript: URL navigation after
LoadDataWithBaseURL. Thus we have to store the base url on
DidStartLoading and use it in DidStopLoading / DidFinishLoad.

We have to preserve the old hack with SetToBaseURLForDataURLIfNeeded
because if the base URL is not valid, DidFinishLoad doesn't
receive it from the renderer.

This change is a short-term workaround. The correct solution
is to pass a flag that the load was through LoadDataWithBaseURL
via Blink, so base URL can be restored correctly within
NavigationController.

BUG=594001
==========

to

==========
Store and use last base URL between DidStart / DidStopLoading

As it turns out that the committed entry may omit the base
URL if there has been a javascript: URL navigation after
LoadDataWithBaseURL. Thus we have to store the base url on
DidStartLoading and use it in DidStopLoading / DidFinishLoad.

We have to preserve the old hack with SetToBaseURLForDataURLIfNeeded
because if the base URL is not valid, DidFinishLoad doesn't
receive it from the renderer.

This change is a short-term workaround. The correct solution
is to pass a flag that the load was through LoadDataWithBaseURL
via Blink, so base URL can be restored correctly within
NavigationController.

BUG=594001

Committed: https://crrev.com/ce1f4d009316d8f5f4c49b18e1bd94687d80c59d
Cr-Commit-Position: refs/heads/master@{#381108}
==========

[commit-bot: I haz the power, 2016-03-14 23:27:06]

Patchset 5 (id:??) landed as
https://crrev.com/ce1f4d009316d8f5f4c49b18e1bd94687d80c59d
Cr-Commit-Position: refs/heads/master@{#381108}