reproed. Test team can you do a bisect to find the regression. please use the attached apk to repro. 

apk with bug

1. open app
2. scroll down
3. click button for open next page.

Result - open next page with scroll dowm, but must top.

Bisected to range: https://chromium.googlesource.com/chromium/src/+log/54.0.2840.18..54.0.2840.21?pretty=fuller&n=10000

Suspected cl: https://codereview.chromium.org/2326983002

Yes that's it.
Thanks.

that has happened on M54. I will look at it today and see what we want to do.

toyoshim@, this is caused by https://codereview.chromium.org/2257743002 and is in fact similar to the data: URL case you had trouble with in our tests.

The issue is that the app is loading the body of the page as a data: URL, but with a base URL (used for history) of about:blank (that's what happens when you pass null). So, even though these two loads are loading *different* data: URLs with different contents, the history URL is the same and so the scroll position restoration kicks in.

It might be sufficient to just make sure we use the real data URL in this case instead of the history URL?

[Bulk edit]

URGENT - PTAL ASAP.

This issue is marked as a stable release blocker for Android M55.  We will be cutting our stable candidate from branch 2883 on Tuesday, 11/29, so *all* blockers must be fixed on trunk by Monday, 11/28 to allow time to merge back to the branch.  Please review this issue ASAP.

Know that this issue shouldn't block the release?  Remove the ReleaseBlock-Stable label.

Unsure if the issue should block the release, or think that the issue should block the release but you know you won't be able to fix it in time?  Please CC me to the bug so that we can discuss options.

Thanks!

Sorry, I was on vacation.
Let me investigate.

What we saw in the layout test is if the data URL page hold completely same contents, the second page load is assumed as a kind of reload. But in this report, the second page contains different data apparently. So, this should be a different problem.

I guess this is because historyUrl isn't set here and both page are assumed as 'about:blank', then the second one is assumed as reload. Let me check how this API is connected to Blink implementation.

Today, I tried creating an android_webview_test to reproduce the problem, but it passes. Probably, this is because the test uses loadDataSync() that calls loadData() internally, and this problem depends on loadDataWithBaseURL().

I will continue investigating this on Thursday since I'm OOO tomorrow.

FYI, this is the test I had today; https://codereview.chromium.org/2525603003/

Update: I succeeded to reproduce this issue in the test.

This problem happens only when embedders call loadDataWithBaseURL() with historyUrl = null or "about:blank". When I passed "https://www.google.com" for historyUrl, it resets scroll positions correctly.

https://cs.chromium.org/chromium/src/android_webview/java/src/org/chromium/android_webview/AwContents.java?dr=CSs&q=loadDataWithBaseURL&sq=package:chromium&l=1536
If baseUrl is null, LoadUrlParams.createLoadDataParamsWithBaseUrl() is called in 'else'.

https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chromium/content_public/browser/LoadUrlParams.java?q=LoadUrlParams&dr=CSs&l=151
If baseUrl is null (or 'about:blank'), historyUrl is set via params.setVirtualUrlForDataUrl().

So, historyUrl is handled as a virtual URL here. In terms of my change, this is a same page navigation, and this behavior change is expected.

But, still it's curious that this happens only if the virtual URL is 'about:blank'.

Also, this problem happens only in the first navigation.
So, root problem seems to exist in the navigation controller rather than reload logic, and reload behavior change just makes problem visible.

Also, now I feel this problem is too minor to block the stable release.
sgurun@, what do you think?

In FrameLoader::determineFrameLoadType(),

m_documentLoader->urlForHistory() holds the specified historyUrl. This looks correct.
But, request.resourceRequest().url() holds the specified baseUrl. I'm not sure this is correct.

Anyway, here we compare previous historyUrl and new baseUrl to identify the same page navigation. This explains why "https://www.google.com" works, but null doesn't.

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebLocalFrameImpl.cpp?rcl=1480047724&l=2002

In WebLocalFrameImpl::loadData(), we setup ResourceRequest as
> 2002  request.setURL(baseURL);

This is just wrong, but we do not have an alternative right way to set base URL at loading time.

So, we have two solutions to be consistent here.


1. Determines same page navigation by FrameLoadRequest.substituteData().failingURL() instead of FrameLoadRequest.resourceRequest().url() for Data URL with baseUrl loading.

This will change behaviors to restore scroll positions for historyURl=null and valid URL.

2. Do not detect same page navigation for Data URL with baseUrl loading.

This will change behaviors not to restore scroll positions for loadDataWithBaseURL() with null or non-data baseUrl.


Probably, the right solution is to compare the current urlForHistory() with the substitute data encoded in data-url format. But since loadDataWithBaseURL() is realized based on multiple tricky hacks to emulate the legacy API, it would be hard to take a right solution.

So, my preference is 2.

Patch for 2. is https://codereview.chromium.org/2531013002/.
I hope this change passes other tests...

vb.belous,

Does this affect something in real applications, or can you imagine some real use cases that could not have any workaround?

I investigated this issue from spec and implementation views, and had some discussion. But at this point, I believe this is an intentional behavior change.

https://developer.android.com/reference/android/webkit/WebView.html#loadDataWithBaseURL(java.lang.String, java.lang.String, java.lang.String, java.lang.String, java.lang.String)

This document says:
 - The base URL is used both to resolve relative URLs and when applying JavaScript's same origin policy
 - Note that the baseUrl is sent in the 'Referer' HTTP header when requesting subresources (images, etc.) of the page loaded using this method.

This actually just reflects how this API is implemented. There, baseUrl isn't just a base URL that you can specify by <base> tag, but it's the page URL actually.

So, in your example, even if you are pushing different contents, these were assumed as reloading the same URL, about:blank, and the second content was just updated.

This works as intended, and just it reflects recent intentional behavior changes on the same page navigation.


If you do not want to restore the scroll position, here are some choices you can take.

1. set window.history.scrollRestoration = "manual" in the first content
2. call window.scrollTo(0, 0) in the second content
3. set different URLs for baseUrl and historyUrl
4. use loadData with <base> tag

If I do not receive any objection in a couple of days, I'd close this as WontFix.

Is it still the case that we're comparing a page URL to a history URL, or was that not actually occurring? Separately for whether the behaviour when both URLs are the same is right, it seems like the same-page navigation detection should either be based on comparing the two page URLs or comparing the two history URLs, and not a mixture of the two..

re#26

Yeah, that looks another troublesome point to me.

Very old WebView document says the last String is "failUrl", and Blink is actually set it to SubstituteData.m_failingUrl.

But, the current WebView document says it's "historyUrl", but implementation still set it to m_failingUrl.

So, I can not know what is expected here.

But, my best guess is that the historyUrl is designed to override the URL for history navigation. So, visiting to another page, and navigating back will show the page in the historyUrl. So using historyUrl to detect the same page navigation looks consistent to me because reload is one of history navigation.

Yes, it does sound like comparing historyUrl would be the right behaviour.

OK, so now we haven't received updates from the original reporter, and sounds like current behavior could be somehow acceptable and reasonable, let me close this report as WontFix (works as intended).

Are you intending to do anything about the comparison of page URL to history URL?

No, I may misunderstood your opinion at #c28.

My understanding is baseUrl is designed to override the page URL for the current navigation, and historyUrl is designed to override the page URL for history navigation in the future.

So, comparing historyUrl of the previous page and baseUrl of the current page sounds correct to me, and would be ideal implementation.

But, from another viewpoint, historyUrl could be undestood as a URL replacement for all history related operations. In such case, we may need a fix, but it could make another minor behavior change because we actually have compared baseUrl and historyUrl for a long time. It has many side effects other than scroll position, e.g. creating an entry in the navigation history if it isn't assumed as a same page navigation.

Hm, I see. OK, let's leave it as it is and see if we get any other bugs.