New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 26 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug-Regression

Blocking:
issue 766255



Sign in to add a comment
link

Issue 794020: webview.canGoBack() always returning false even with web history

Reported by robertle...@gmail.com, Dec 12 2017

Issue description

Steps to reproduce the problem:
1. Create a fragment with a webview inside
2. navigiate a few sites deep in the webview building a backstack
3. Try using webview.canGoBack() api. Observe it always returns false 

What is the expected behavior?
webview.canGoBack() should return true if the webview history stack is larger than 1 and false otherwise

What went wrong?
Method always returns false

Did this work before? Yes the webview implementation in chrome 62

Does this work in other browsers? Yes

Chrome version: <Copy from: 'about:version'>  Channel: stable
OS Version: 6.0, 7.0, 8.0
Flash Version: Shockwave Flash 25.0 r0

This seems to happen on chromium 63 and above. I can reproduce it on the chrome beta and chrome dev versions as well. I tested by using the webview implementation in developer options. 
Specific problem version tested: 63.0.3239.83
Last working version tested: 62.0.3202.84
 
Showing comments 21 - 120 of 120 Older

Comment 21 by robertle...@gmail.com, Dec 13 2017

I will work on this. Most likely tomorrow though.

Comment 22 by robertle...@gmail.com, Dec 13 2017

I may not be understanding but in my example a.com *DOES* redirect to a.redir.com but this is an intermediary page that immediately redirects to b.com. 

a.redir.com has a 302 http status code and a.com and b.com have a 200 http status code.

Comment 23 by ntfschr@chromium.org, Dec 13 2017

Thanks! I haven't had luck so far, but I've tried:

shouldOverrideUrlLoading(view, request) {
  if (request.isRedirect) {
     view.loadUrl(request.getUrl().toString());
     return true;
  }
  return false;
}

onCreate() {
  // attach to view hierarchy, set webviewclient, etc.
  // ...

  view.loadUrl("https://httpbin.org/");
}

onBackPressed() {
  if (canGoBack()) goBack();
  else super.onBackPressed();
}

// User action: after loading httpbin.org, click on "/redirect-to?url=foo". Then click 'back'

Comment 24 by arthurso...@chromium.org, Dec 13 2017

I have trouble understanding what happens.
I read comment #14 and here is what I understood.
(I added the implicit calls to shouldOverrideUrl where I think you returned false)

````
Navigation 1:
* loadUrl(a.com)
* shouldOverrideUrl(a.com, isRedirect = false)   [ return false ]
* a.com is loaded.

Navigation 2:
* User click on a.redir.com
* shouldOverrideUrl(a.redir.com, isRedirect = false) [ loadUrl(a.redir.com); return true ]
* shouldOverrideUrl(a.redir.com, isRedirect = false) [ return false ]
* shouldOverrideUrl(b.com      , isRedirect = true)  [ loadUrl(b.com); return true ]
* shouldOverrideUrl(b.com      , isRedirect = false) [ return false ]
* b.com is loaded.

Then webview.canGoBack() returns false instead of true. If it is the case, that's a bug.

```
Is it correct?

In comment #22, you wrote that a.com has a 200 HTTP status code, so I think you wanted to say that a.com *DOES NOT* redirect to a.redir.com.

Comment 25 by frederik...@apploft.de, Dec 13 2017

I made a small project to reproduce the issue (tested with emulator x86 Api 26 and Chrome 63):

the sample app loads stackoverflow, follow steps to reproduce the issue: 
1. navigate into a question
2. navigate to user profile
3. press back
= backForwardList is of size 1, leading to canGoBack returning false
WebViewIssueCanGoBack.zip
131 KB Download

Comment 26 by clamy@chromium.org, Dec 13 2017

Well your test case returns true on all ShouldOverrideURLLoading so it can't navigate. We also call this function on loads you initiated from WebView, so they get cancelled as well.

Comment 27 by arthursonzogni@google.com, Dec 13 2017

FYI: the WebViewClient implementation in comment 25

```
class SimpleWebViewClient : WebViewClient() {
  override fun shouldOverrideUrlLoading(view: WebView?, url: String?): Boolean {
  view?.let {
    it.loadUrl(url)
  }
  return true
}
```

I am a surprise that you are able to navigate anywhere (see comment 26). I will take a look and try to understand how it is possible.

Comment 28 by arthurso...@chromium.org, Dec 13 2017

Cc: arthurso...@chromium.org jam@chromium.org ahemery@chromium.org nasko@chromium.org clamy@chromium.org

Comment 29 by arthurso...@chromium.org, Dec 13 2017

Components: UI>Browser>Navigation
Labels: Proj-PlzNavigate
Owner: arthurso...@chromium.org
Status: Assigned (was: Unconfirmed)
Thanks for the repro apk. I can reproduce the issue.

I tried to use the old navigation path (--disable-browser-side-navigation) and I can't reproduce.

I understand why in this example it is still able to navigate somewhere even if every URLs are overridden. It is because every renderer-initiated navigation are turned into a browser-initiated ones and the browser-initiated ones returns always false in ShouldOverrideURLLoading (See https://cs.chromium.org/chromium/src/android_webview/browser/aw_content_browser_client.cc?type=cs&l=614)

I think there is a bug.

Comment 30 by arthurso...@chromium.org, Dec 13 2017

Cc: boliu@chromium.org

Comment 31 by arthurso...@chromium.org, Dec 13 2017

I think I found something. The renderer-initiated navigation is canceled and replaced by a browser-initiated one with the same url.
Then NavigationControllerImpl::RendererDidNavigateToSamePage is called.

Here are some logs of interesting methods in NavigationRequest and NavigationControllerImpl.
```
# First navigation
[ERROR:navigation_controller_impl.cc(259)] NavigationControllerImpl
[ERROR:navigation_controller_impl.cc(681)] LoadURLWithParams
[ERROR:navigation_controller_impl.cc(188)] CreateNavigationEntry
[ERROR:navigation_controller_impl.cc(446)] LoadEntry
[ERROR:navigation_controller_impl.cc(466)] SetPendingEntry
[ERROR:navigation_controller_impl.cc(2234)] DiscardNonCommittedEntriesInternal
[ERROR:navigation_controller_impl.cc(2240)] DiscardPendingEntry
[ERROR:navigation_controller_impl.cc(1943)] NavigateToPendingEntry
[ERROR:navigation_controller_impl.cc(2057)] NavigateToPendingEntryInternal
[ERROR:navigation_request.cc(224)] CreateBrowserInitiated url = https://stackoverflow.com/
[ERROR:navigation_request.cc(357)] NavigationRequest url = https://stackoverflow.com/
[ERROR:navigation_request.cc(443)] BeginNavigation url = https://stackoverflow.com/
[ERROR:navigation_request.cc(453)] ShouldOverrideURLLoading (begin) url = https://stackoverflow.com/ my_id = 1
[ERROR:navigation_request.cc(461)] ShouldOverrideURLLoading (end) my_id = 1 override = 0 this_ptr = 1
[ERROR:navigation_request.cc(559)] CreateNavigationHandle url = https://stackoverflow.com/
[ERROR:navigation_request.cc(988)] OnStartChecksComplete url = https://stackoverflow.com/
[ERROR:navigation_request.cc(975)] OnRequestStarted url = https://stackoverflow.com/
[ERROR:navigation_request.cc(770)] OnResponseStarted url = https://stackoverflow.com/
[ERROR:navigation_request.cc(1154)] OnWillProcessResponseChecksComplete url = https://stackoverflow.com/
[ERROR:navigation_request.cc(1233)] CommitNavigation url = https://stackoverflow.com/
[ERROR:navigation_request.cc(438)] ~NavigationRequest url = https://stackoverflow.com/

# Second and third navigations.
[ERROR:navigation_controller_impl.cc(823)] RendererDidNavigate
[ERROR:navigation_controller_impl.cc(1110)] RendererDidNavigateToNewPage
[ERROR:navigation_controller_impl.cc(2234)] DiscardNonCommittedEntriesInternal
[ERROR:navigation_controller_impl.cc(2240)] DiscardPendingEntry
[ERROR:navigation_controller_impl.cc(2234)] DiscardNonCommittedEntriesInternal
[ERROR:navigation_controller_impl.cc(2240)] DiscardPendingEntry
[ERROR:navigation_controller_impl.cc(2161)] NotifyNavigationEntryCommitted
[ERROR:navigation_controller_impl.cc(2216)] NotifyEntryChanged
[ERROR:navigation_controller_impl.cc(188)] CreateNavigationEntry
[ERROR:navigation_controller_impl.cc(466)] SetPendingEntry
[ERROR:navigation_controller_impl.cc(2234)] DiscardNonCommittedEntriesInternal
[ERROR:navigation_controller_impl.cc(2240)] DiscardPendingEntry
[ERROR:navigation_request.cc(294)] CreateRendererInitiated url = https://stackoverflow.com/questions/47795219/urlrewrite-rule-to-ignore-any-character-but-digit
[ERROR:navigation_request.cc(357)] NavigationRequest url = https://stackoverflow.com/questions/47795219/urlrewrite-rule-to-ignore-any-character-but-digit
[ERROR:navigation_request.cc(443)] BeginNavigation url = https://stackoverflow.com/questions/47795219/urlrewrite-rule-to-ignore-any-character-but-digit
[ERROR:navigation_request.cc(453)] ShouldOverrideURLLoading (begin) url = https://stackoverflow.com/questions/47795219/urlrewrite-rule-to-ignore-any-character-but-digit my_id = 2
[ERROR:navigation_controller_impl.cc(681)] LoadURLWithParams
[ERROR:navigation_controller_impl.cc(188)] CreateNavigationEntry
[ERROR:navigation_controller_impl.cc(446)] LoadEntry
[ERROR:navigation_controller_impl.cc(466)] SetPendingEntry
[ERROR:navigation_controller_impl.cc(2234)] DiscardNonCommittedEntriesInternal
[ERROR:navigation_controller_impl.cc(2240)] DiscardPendingEntry
[ERROR:navigation_controller_impl.cc(1943)] NavigateToPendingEntry
[ERROR:navigation_controller_impl.cc(2057)] NavigateToPendingEntryInternal
[ERROR:navigation_request.cc(224)] CreateBrowserInitiated url = https://stackoverflow.com/questions/47795219/urlrewrite-rule-to-ignore-any-character-but-digit
[ERROR:navigation_request.cc(357)] NavigationRequest url = https://stackoverflow.com/questions/47795219/urlrewrite-rule-to-ignore-any-character-but-digit
[ERROR:navigation_request.cc(438)] ~NavigationRequest url = https://stackoverflow.com/questions/47795219/urlrewrite-rule-to-ignore-any-character-but-digit
[ERROR:navigation_request.cc(461)] ShouldOverrideURLLoading (end) my_id = 2 override = 1 this_ptr = 0
[ERROR:navigation_request.cc(443)] BeginNavigation url = https://stackoverflow.com/questions/47795219/urlrewrite-rule-to-ignore-any-character-but-digit
[ERROR:navigation_request.cc(453)] ShouldOverrideURLLoading (begin) url = https://stackoverflow.com/questions/47795219/urlrewrite-rule-to-ignore-any-character-but-digit my_id = 3
[ERROR:navigation_request.cc(461)] ShouldOverrideURLLoading (end) my_id = 3 override = 0 this_ptr = 1
[ERROR:navigation_request.cc(559)] CreateNavigationHandle url = https://stackoverflow.com/questions/47795219/urlrewrite-rule-to-ignore-any-character-but-digit
[ERROR:navigation_request.cc(988)] OnStartChecksComplete url = https://stackoverflow.com/questions/47795219/urlrewrite-rule-to-ignore-any-character-but-digit
[ERROR:navigation_request.cc(975)] OnRequestStarted url = https://stackoverflow.com/questions/47795219/urlrewrite-rule-to-ignore-any-character-but-digit
[ERROR:navigation_request.cc(770)] OnResponseStarted url = https://stackoverflow.com/questions/47795219/urlrewrite-rule-to-ignore-any-character-but-digit
[ERROR:navigation_request.cc(1154)] OnWillProcessResponseChecksComplete url = https://stackoverflow.com/questions/47795219/urlrewrite-rule-to-ignore-any-character-but-digit
[ERROR:navigation_request.cc(1233)] CommitNavigation url = https://stackoverflow.com/questions/47795219/urlrewrite-rule-to-ignore-any-character-but-digit
[ERROR:navigation_request.cc(438)] ~NavigationRequest url = https://stackoverflow.com/questions/47795219/urlrewrite-rule-to-ignore-any-character-but-digit
[ERROR:navigation_controller_impl.cc(2216)] NotifyEntryChanged
[ERROR:navigation_controller_impl.cc(823)] RendererDidNavigate
[ERROR:navigation_controller_impl.cc(1422)] RendererDidNavigateToSamePage
[ERROR:navigation_controller_impl.cc(1854)] DiscardNonCommittedEntries
[ERROR:navigation_controller_impl.cc(2234)] DiscardNonCommittedEntriesInternal
[ERROR:navigation_controller_impl.cc(2240)] DiscardPendingEntry
[ERROR:navigation_controller_impl.cc(2234)] DiscardNonCommittedEntriesInternal
[ERROR:navigation_controller_impl.cc(2240)] DiscardPendingEntry
[ERROR:navigation_controller_impl.cc(2161)] NotifyNavigationEntryCommitted
[ERROR:navigation_controller_impl.cc(2216)] NotifyEntryChanged

# Go back (crash)
[ERROR:navigation_controller_impl.cc(597)] GoBack
[ERROR:navigation_controller_impl.cc(609)] GoToIndex
[FATAL:navigation_controller_impl.cc(613)] Check failed: false. 
```

Comment 32 by pauljensen@chromium.org, Dec 13 2017

 Issue 794511  has been merged into this issue.

Comment 33 by clamy@chromium.org, Dec 13 2017

I think the issue is also that we don't get this block
[ERROR:navigation_controller_impl.cc(2216)] NotifyEntryChanged
[ERROR:navigation_controller_impl.cc(823)] RendererDidNavigate
for the first navigation. That means we don't create an history item for the first navigation.

Comment 34 by clamy@chromium.org, Dec 13 2017

Ok that's me misreading the file. The issue is that we treat the third navigation as same page which is weird. We're still looking at what's happening.

Comment 35 by torne@chromium.org, Dec 13 2017

Re bo's comment 15: usually what apps want is for custom headers to persist across navigations in general (at least within a single domain, but sometimes across domains as well). We could conceivably look at preserving the custom headers for redirects if we don't do that already, and I'm not sure whether that would be sufficient for this particular case or not, but there are definitely apps that will still want to cancel all renderer-initiated navigations and reissue them as browser-initiated navigations with custom headers even if redirects work "as expected".

The custom-headers-in-loadUrl API as-is is rarely fit for purpose :/

This is one of the reasons why we proposed the webRequest-like API for modifying the network stack behaviour, but it was dropped from scope for this android release..

Comment 36 by torne@chromium.org, Dec 13 2017

(custom headers not being used for subresources is another problem with how this API currently works)

Comment 37 by clamy@chromium.org, Dec 13 2017

Cc: creis@chromium.org
+creis for history question
Update on the issue: we have identified the problem with the test case. If you resubmit the same URL we treat this as a reload in the condition to convert enter in omnibox for reloads: https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_controller_impl.cc?dr=CSs&l=1962. I think it's wrong to use a non-committed navigation in that comparison: how is this even working in the first place?

However that doesn't explain the redirect issue if it applies to server redirects.
@torne, bo: does WebView differentiates between server and client redirects?

Comment 39 by clamy@chromium.org, Dec 13 2017

Also we think it worked pre-PlzNavigate because shouldOverrideURLLoading was called before we created a pending NavigationEntry for the load that would be cancelled. So we didn't have the issue with the new navigation matching the previous cancelled one. I doubt it would have worked if the navigation was browser-initiated.

Comment 40 by torne@chromium.org, Dec 13 2017

There's not actually any such thing as a client redirect. What's being referred to here is when the app chooses to respond to shouldOverrideUrlLoading by calling loadUrl (which starts another navigation) and returning true (which cancels the current navigation).

The *intent* of the app developer here is a redirection, but this isn't handled any differently than any other navigation sequence by WebView.

Probably the intent would be expressed more clearly if there was a simple way to cancel the current navigation *first* and then start the new navigation, but since the navigation cancellation is expressed by returning true from the callback there's no way for the app to do this without posting a task to itself or some other gross asynchronous thing..

Comment 41 by clamy@chromium.org, Dec 13 2017

Ah that makes sense, thanks!

Comment 42 by torne@chromium.org, Dec 13 2017

(also since not all of these things are entirely synchronous or happening on one thread there's already a bunch of weird ordering involved)

Comment 43 by arthurso...@chromium.org, Dec 13 2017

I think we could disable "Convert Omnibox-to-enter into reload" for entries that are created by WebView:
https://chromium-review.googlesource.com/c/chromium/src/+/824682

I tried it on the repro apk. It works. Maybe there are better way to fix it thought.

Comment 44 by hello.ch...@gmail.com, Dec 13 2017

(I read through the full discussion above about redirects, but in addition:)

This violates a basic invariant that one would expect, which is that if WebView.copyBackForwardList().getSize() is > 0, then canGoBack() should return true.

Pointing this out in case the issue is more widespread than simply in the case of redirects.

Comment 45 by boliu@chromium.org, Dec 13 2017

> This violates a basic invariant that one would expect, which is that if WebView.copyBackForwardList().getSize() is > 0, then canGoBack() should return true.

That doesn't make sense to me. If size is 1, then I don't expect canGoBack to return true. If size is n > 1 but the current index is 0, then canGoBack should return false.

Comment 46 by boliu@chromium.org, Dec 13 2017

re #27

> I am a surprise that you are able to navigate anywhere (see comment 26). I will take a look and try to understand how it is possible.

shouldOverrideUrlLoading isn't called on an app initiated loadUrl navigations (this doesn't hold for the other calls though, like loadData!!)

Comment 47 by hello.ch...@gmail.com, Dec 13 2017

> > This violates a basic invariant that one would expect, which is that if WebView.copyBackForwardList().getSize() is > 0, then canGoBack() should return true.

> That doesn't make sense to me. If size is 1, then I don't expect canGoBack to return true. If size is n > 1 but the current index is 0, then canGoBack should return false.

You’re right, I didn’t consider that possibility. Upon investigating further, it seems like the internal history list itself is not correctly maintained.

Comment 48 by jam@chromium.org, Dec 13 2017

Labels: -M-63 M-64
Based on discussions with Camille, Torne, Bo and others, since:
1) we don't have reports of major apps broken
2) there's a workaround of posting the navigation asynchronously
3) a fix may have other unintended regressions
4) a fix would delay the respin with the fixes for  bug 793648  and bug 793432
this doesn't have to be fixed in m63, and we can wait till m64

Comment 49 by clamy@chromium.org, Dec 13 2017

Based on the issue, posting the navigation asynchronously may not work. What you want to do is loading anything that has not the same URL (eg about:blank) just before what you actually want to load.

Comment 50 by robertle...@gmail.com, Dec 13 2017

Just to add after doing some more testing this sometimes happens without the redirects as well that was also causing problems. 

Scenario:
1. Set up we already have a page loaded by calling loadUrl() there are a few redirects to get to the initial page(). We always call loadUrl() and return true for these redirects. Call this final page a.com

2. User clicks a link for b.com. There are no redirects in this case, shouldOverrideUrlLoading() is called for b.com. We create a set of headers and call loadUrl(b.com, headers) and return true. 

In this case, b.com is loaded and calling canGoBack() returns false.

FYI: a.com and b.com are different pages in the same domain if that matters.

Comment 51 by jarev...@bodas.net, Dec 14 2017

Hi, Same problem for our application:

Steps to reproduce:
(1) You need a Android Studio project with uses Webview and pass custom headers
(2) Override shouldOverrideUrlLoading and call myWebView.loadUrl(url, customHeaders);

Expected result:

Custom Headers are sent to the server in each request. When you navigate in the webpage, myWebView.copyBackForwardList() grows, you can click back buttons, etc.

Actual result:

Back buttons doesn't work. myWebView.canGoBack() always return false. myWebView.copyBackForwardList().getCurrentIndex() doesn't grow if you navigate.

This feature works perfectly until Android System Webview 62 (included).

As you say, it will be fixed in release 64?

Comment 52 by arthurso...@chromium.org, Dec 14 2017

comment 50: Thank you for clarifying the point with redirects. I can now say that this bug and redirects are not related. The issue happens in the second navigation (b.com). When chrome is asked to navigate twice to b.com, it misclassifies it as a reload.

Comment 53 by bugdroid1@chromium.org, Dec 14 2017

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7a8243685145cb1a6ba64916ec27487b9c0c850d

commit 7a8243685145cb1a6ba64916ec27487b9c0c850d
Author: arthursonzogni <arthursonzogni@chromium.org>
Date: Thu Dec 14 16:41:42 2017

Disable "Convert Enter-in-omnibox to a reload" for webview.

This CL disable "Convert Enter-in-omnibox to a reload" when the
navigation entry are created by WebView.

Some WebView initiated navigation were misclassified as "reload".

Bug:  794020 
Change-Id: I443bc00601c33c94005aec0eb4f1592f83b6f64b
Reviewed-on: https://chromium-review.googlesource.com/824682
Reviewed-by: Camille Lamy <clamy@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524086}
[modify] https://crrev.com/7a8243685145cb1a6ba64916ec27487b9c0c850d/content/browser/frame_host/navigation_controller_impl.cc

Comment 54 by pauljensen@chromium.org, Dec 14 2017

 Issue 794833  has been merged into this issue.

Comment 55 by pauljensen@chromium.org, Dec 14 2017

 Issue 794834  has been merged into this issue.

Comment 56 by arthurso...@chromium.org, Dec 14 2017

Status: Fixed (was: Assigned)
Patch in comment 53 fixed the issue (verified locally).

Comment 57 by bugdroid1@chromium.org, Dec 14 2017

Project Member
Labels: merge-merged-3239
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/463b24bd7383e834ed71374d71326209ab234d62

commit 463b24bd7383e834ed71374d71326209ab234d62
Author: arthursonzogni <arthursonzogni@chromium.org>
Date: Thu Dec 14 19:30:49 2017

Disable "Convert Enter-in-omnibox to a reload" for webview.

This CL disable "Convert Enter-in-omnibox to a reload" when the
navigation entry are created by WebView.

Some WebView initiated navigation were misclassified as "reload".

Bug:  794020 
Change-Id: I443bc00601c33c94005aec0eb4f1592f83b6f64b
Reviewed-on: https://chromium-review.googlesource.com/824682
Reviewed-by: Camille Lamy <clamy@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#524086}(cherry picked from commit 7a8243685145cb1a6ba64916ec27487b9c0c850d)
Reviewed-on: https://chromium-review.googlesource.com/827566
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/branch-heads/3239@{#679}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[modify] https://crrev.com/463b24bd7383e834ed71374d71326209ab234d62/content/browser/frame_host/navigation_controller_impl.cc

Comment 58 by bugdroid1@chromium.org, Dec 14 2017

Project Member
Labels: merge-merged-3282
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/657d383372e475c768fc2232e967a7682dacc4c8

commit 657d383372e475c768fc2232e967a7682dacc4c8
Author: arthursonzogni <arthursonzogni@chromium.org>
Date: Thu Dec 14 19:31:35 2017

Disable "Convert Enter-in-omnibox to a reload" for webview.

This CL disable "Convert Enter-in-omnibox to a reload" when the
navigation entry are created by WebView.

Some WebView initiated navigation were misclassified as "reload".

Bug:  794020 
Change-Id: I443bc00601c33c94005aec0eb4f1592f83b6f64b
Reviewed-on: https://chromium-review.googlesource.com/824682
Reviewed-by: Camille Lamy <clamy@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#524086}(cherry picked from commit 7a8243685145cb1a6ba64916ec27487b9c0c850d)
Reviewed-on: https://chromium-review.googlesource.com/827568
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/branch-heads/3282@{#232}
Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840}
[modify] https://crrev.com/657d383372e475c768fc2232e967a7682dacc4c8/content/browser/frame_host/navigation_controller_impl.cc

Comment 59 by bugdroid1@chromium.org, Dec 14 2017

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4331f740f098af8771f625ca11a40cfc2b8e2777

commit 4331f740f098af8771f625ca11a40cfc2b8e2777
Author: John Abd-El-Malek <jam@chromium.org>
Date: Thu Dec 14 21:20:47 2017

Revert "Disable "Convert Enter-in-omnibox to a reload" for webview."

This reverts commit 463b24bd7383e834ed71374d71326209ab234d62.

Reason for revert: The major app that reported this isn't seeing it anymore, so reverting this cl per discussions.

Original change's description:
> Disable "Convert Enter-in-omnibox to a reload" for webview.
> 
> This CL disable "Convert Enter-in-omnibox to a reload" when the
> navigation entry are created by WebView.
> 
> Some WebView initiated navigation were misclassified as "reload".
> 
> Bug:  794020 
> Change-Id: I443bc00601c33c94005aec0eb4f1592f83b6f64b
> Reviewed-on: https://chromium-review.googlesource.com/824682
> Reviewed-by: Camille Lamy <clamy@chromium.org>
> Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
> Cr-Original-Commit-Position: refs/heads/master@{#524086}(cherry picked from commit 7a8243685145cb1a6ba64916ec27487b9c0c850d)
> Reviewed-on: https://chromium-review.googlesource.com/827566
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Cr-Commit-Position: refs/branch-heads/3239@{#679}
> Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}

TBR=jam@chromium.org,arthursonzogni@chromium.org

Change-Id: I7f7347484bf2a2f4bea951de797f8d2aa1e49c65
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  794020 
Reviewed-on: https://chromium-review.googlesource.com/827570
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/branch-heads/3239@{#681}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[modify] https://crrev.com/4331f740f098af8771f625ca11a40cfc2b8e2777/content/browser/frame_host/navigation_controller_impl.cc

Comment 60 by ntfschr@chromium.org, Dec 15 2017

Cc: ppolise...@chromium.org
I've built a repro APK.

Steps:
1. Clear all webview flags (default behavior)
2. Open the app (it opens to google.com)
3. Search for something (this takes you to a new page)
4. Click the Android back button

Expected results:
4. The back button should navigate backward

Observed results (without the patch)
4. The app creates a toast that says "WebView cannot go back", even though there should be a previous page in the history
PlzNavGoBackBug.apk
1.3 MB Download

Comment 61 by ntfschr@chromium.org, Dec 15 2017

We got another external report of this at https://b.corp.google.com/issues/70698550 (public buganizer)

Comment 62 by phistuck@gmail.com, Dec 15 2017

#61 - here is a public link for the report - https://issuetracker.google.com/issues/70698550

Comment 63 by ppolise...@chromium.org, Dec 15 2017

Fix verified with apk & steps in #60, on 64.0.3282.31 & 65.0.3295.0, tested on Pixel 2XL/OPM1.171019.013, back button navigates to the previous page.

Comment 64 by robertle...@gmail.com, Dec 16 2017

I took a look at the CR and the fix looks relatively safe. Is there any plan to port this over as a patch to the m63 release since this is starting to affect more and more applications?

Comment 65 by clamy@chromium.org, Dec 18 2017

There's currently no plan to merge this in M63.

Comment 66 by ade...@gmail.com, Dec 18 2017

This bug breaks application navigation on some android apps that use WebView, users cannot go back to previous page, instead the application closes.
e.g. https://play.google.com/store/apps/details?id=cinemagia.mobile
And this happened with zero warning to app developers as it is a chrome update.
I don't understand how chrome team can ignore a hot-fix for this issue.

Comment 67 by ntfschr@chromium.org, Dec 18 2017

As far as we know, this issue only affects 2 apps for sure (cinemagia.mobile and kr.co.saramin.brandapp). If we're mistaken, then external app developers should *speak up now* and post links to their apps in the play store so we can assess impact.

At this point, the only advice I can give to app developers is to recommend users switch to WebView beta (which is currently at M63, but will soon update to M64 with the bug fix). Or better yet, reevaluate if the shouldOverrideUrlLoading+loadUrl combo is really necessary at this point.

---

It'd be great if app developers could explain the motivation for this shouldOverrideUrlLoading+loadUrl pattern. Is this only for custom headers on redirects and user clicks? Are there other reasons? We would be happy to work with developers to create a better API.

Comment 68 by hello.ch...@gmail.com, Dec 18 2017

> *speak up now* and post links to their apps in the play store so we can assess impact.

I intentionally did not add a “me too!” post once the original bug was identified to avoid adding noise, but if you’re actually looking for me-too’s as a signal, then here you go:

App Affected:
https://play.google.com/store/apps/details?id=com.chimbori.hermitcrab

Motivation:
Passing custom headers on regular user clicks, specifically: adding "DNT" (do not track) and "Save-Data" headers.

Our current work-around is to avoid sending these headers (return false; in shouldOverrideUrlLoading) to M63 and M64, which breaks a promise we made to our users.

Comment 69 by ntfschr@chromium.org, Dec 18 2017

Thanks hello.chimbori@! Yes, in this case, posting a play store link is a great signal.

Comment 70 by ade...@gmail.com, Dec 18 2017

ntfschr@ - for cinemagia.mobile we use shouldOverrideUrlLoading+loadUrl to replace some web pages with native UI displayed on top of the WebView for a better user experience; we modify the URL so it points to an internal dummy page; we want it in browser history to be able to navigate back and forward and seamlessly switch between native UI and web pages.

Comment 71 by scott13...@gmail.com, Dec 18 2017

Our app is built around a web view and will not function at all because of this bug.

https://play.google.com/store/apps/details?id=sa.android

How long will it be before the fix is available? This bug came out of nowhere. We were notified by this because of a user telling us about it! After being informed, I had two tablets side by side, one exhibited the bug, and the other didn't. Then the other pulled the latest Chrome, and started exhibiting the bug.

Comment 72 by ntfschr@chromium.org, Dec 18 2017

Blocking: 766255

Comment 73 by girdhari...@gmail.com, Dec 19 2017

> *speak up now* and post links to their apps in the play store so we can assess impact.

I intentionally did not add a “me too!” post once the original bug was identified to avoid adding noise, but if you’re actually looking for me-too’s as a signal, then here you go:

App Affected:
https://play.google.com/store/apps/details?id=com.commutree

Comment 74 by frederik...@apploft.de, Dec 19 2017

Bug does not break the app but leads to unexpected navigation issues

Affected App:

https://play.google.com/store/apps/details?id=de.bonprix

Comment 75 by branimir...@undabot.com, Dec 19 2017

This affects the app navigation and results in bad reviews. We rely on `canGoBack` to setup the back button in the native Toolbar to provide back navigation. With this bug, the app behaves as if it is on the root webView screen all the time, and pressing back button closes the app :/

shouldOverrideUrlLoading+loadUrl pattern is used to inject headers on each load.

I think this is pretty serious bug, because there are a lot of apps that use WebView, many of which are considered "done" and can not afford to reach to developers to work around it.

Me personally am thinking about implementing a hack, because I don't see a fix from your side coming any soon, and the bug affects the app a lot.
Instead of using `canGoBack`, I might check if the currently loaded URL corresponds to home URL - this way I can know if I'm on the root screen or not.

App affected:
https://play.google.com/store/apps/details?id=com.undabot.android.bolha&hl=en

Comment 76 by jarev...@bodas.net, Dec 19 2017

Our problem is exactly the same that #75.
We've a lot of apps in Google Play. We don't send the custom headers and append this information in the user-agent.

It's an ugly workaround, but in our case is solved temporally.

For us, has been a critical bug.

Comment 77 by almardie...@gmail.com, Dec 19 2017

We are also seeing this bug. We create a lot of simple webview apps (like Webclips) for our customers and distribute them through an MDM solution. So these are apps that are not being installed via the PlayStore. All these apps are using 'canGoBack' meaning that all are now broken as for the navigation.

Plz fix asap.

Comment 78 by vb.k...@gmail.com, Dec 19 2017

My app is also affected by the issue.

shouldOverrideUrlLoading+loadUrl pattern is used to send user defined headers

App affected:
https://play.google.com/store/apps/details?id=idm.internet.download.manager

Comment 79 Deleted

Comment 80 by tommiepo...@gmail.com, Dec 19 2017

We're seeing this issue on a browser with more than quarter of a million daily users. And it's currently tanking our Google Play rating. Please advice!

And we second this:
"shouldOverrideUrlLoading+loadUrl pattern is used to send user defined headers"

Comment 81 by nasko@chromium.org, Dec 19 2017

Cc: cma...@chromium.org
Adding cmasso@ for visibility.

Comment 82 by ntfschr@chromium.org, Dec 19 2017

Thanks to all the external developers who shared their play store links!

And also thanks for explaining the use-cases. It sounds like the major (but not the only) use-case is to send headers on *every* URL load, not just the initial load.

Comment 83 by vygen.ja...@gmail.com, Dec 20 2017

Our app is also affected by this issue.
shouldOverrideUrlLoading+loadUrl pattern is used to send user defined headers

App affected:
https://play.google.com/store/apps/details?id=de.kicktipp.mbookmark
We have 2 Million Downloads! I don't know if this is a lot. But for us and our users the bug is very annoying. Please fix it as soon as possible.

Comment 84 by cma...@chromium.org, Dec 20 2017

The fix provided did not fix this issue? Can the bug be re-opened if that's the case?

Comment 85 by nasko@chromium.org, Dec 20 2017

The underlying issue is fixed, but the fix is only present in M65 and M64, not in M63. Therefore the bug resolution as fixed is correct.

Comment 86 by devbisc...@gmail.com, Dec 22 2017

Hi, nasko. My app is also affected by this issue.
When I tried to test with chrome dev version(65), I verified the fix was included.
But In chrome beta(64), It also has the same issue. 

The fix will be release in 64(Beta, also stable), right? 
Could you let me know ETA for the update?

Comment 87 by ntfschr@chromium.org, Dec 22 2017

Yes, the fix will be released in M64 beta. It missed the first M64 beta, but should be in all future M64 releases (multiple betas followed by a stable).

Comment 88 by devbisc...@gmail.com, Dec 22 2017

Thanks for quick update. 
I hope it will be released soon.

Comment 89 Deleted

Comment 90 by sehho...@gmail.com, Dec 26 2017

It seems loadUrl is not working well with Chrome 63 webview.
Instead of calling loadUrl and return true/false from within shouldOverrideUrlLoading.

Try using:
return super.shouldOverrideUrlLoading (view, url);

in shouldOverrideUrlLoading(final WebView view, final String url) { ... }

Comment 91 by nguyenli...@gmail.com, Dec 28 2017

I have problem canGoBack() return false in version 63.
I hope it will be released soon.
Thanks.

Comment 92 by tommiepo...@gmail.com, Dec 28 2017

Any kind of release-timeline would be highly appreciated. This issue is snowballing out of control for us as more and more users get updated to the broken version. Our ratings are taking a huge hit and it's hard for us to say "Yeah, Google has fixed it. But we don't know when it'll be released".

Any type of communication would be appreciated, thanks! :)

Comment 93 by ntfschr@chromium.org, Dec 28 2017

This has been fixed in M64. You can find a public release calendar here [1]. This is just an approximate schedule, but we aim to be close to the public schedule.

According to the schedule, M64 will go to stable January 23rd.

While M64 is currently in beta, we have not yet made a beta release with the fix. I do not have information for when such a beta release will go out (but it will be sooner than Jan 23rd).

[1] https://www.chromium.org/developers/calendar

Comment 94 by ntfschr@chromium.org, Dec 28 2017

I just checked - our current dev release also contains the fix. So users could switch their WebView implementation to Chrome Dev if necessary. Instructions (only available for N+):

https://www.chromium.org/developers/androidwebview/android-webview-beta

Comment 95 by kaiyang...@gmail.com, Dec 30 2017

Please fix this a.s.a.p. It is ruining the app using webview.

Comment 96 by torne@chromium.org, Jan 2 2018

Cc: sandeepkumars@chromium.org
 Issue 797857  has been merged into this issue.

Comment 97 by ctzsm@chromium.org, Jan 2 2018

Cc: ctzsm@chromium.org
 Issue 795702  has been merged into this issue.

Comment 98 by ppolise...@chromium.org, Jan 2 2018

Fix verified with apk & steps in #60, on 64.0.3282.67, tested on Pixel 2XL/OPM1.171019.018 & Nexus 6P/N2G48I, back button navigates to the previous page.

Comment 99 by ntfschr@chromium.org, Jan 9 2018

FYI the latest beta release contains the fix (instructions linked in comment #94).

Comment 100 by bugdroid1@chromium.org, Jan 9 2018

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/983a483cfa2c2c6662190d8ff58316244a573067

commit 983a483cfa2c2c6662190d8ff58316244a573067
Author: Nate Fischer <ntfschr@chromium.org>
Date: Tue Jan 09 22:39:49 2018

AW: regression test for loadUrl within shouldOverrideUrlLoading

No change to production logic, this only tests an already-fixed
scenario.

This tests a common pattern for apps to use: cancel a navigation in
shouldOverrideUrlLoading, but reload the same URL with a call to
loadUrl() (this is usually done to append custom headers to render
initiated navigations, but it reproduces with a simple loadUrl() too).

Applications previously observed that the back-forward history breaks,
since the canceled-and-reloaded URL doesn't appear as a history entry.
This test ensures that canGoBack() and goBack() behave sensibly.

Bug:  794020 
Test: run_webview_instrumentation_test_apk -f AwContentsClientShouldOverrideUrlLoadingTest#testReloadingUrlDoesNotBreakBackForwardList
Change-Id: I51aa11d01d47d8e23f4dc7a35478d20a4354e45a
Reviewed-on: https://chromium-review.googlesource.com/855643
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528142}
[modify] https://crrev.com/983a483cfa2c2c6662190d8ff58316244a573067/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java

Comment 101 by scvyong...@gmail.com, Jan 11 2018

I wasted time to solve this problem for 2 days....
I really hope it will be released soon.

Comment 102 by sunjays...@gmail.com, Jan 13 2018

I have trouble accepting that google deals with their products like this. It's broken. 

"At this point, the only advice I can give to app developers is to recommend users switch to WebView beta (which is currently at M63, but will soon update to M64 with the bug fix). Or better yet, reevaluate if the shouldOverrideUrlLoading+loadUrl combo is really necessary at this point."

This isn't proper advice.

This is immature. The change broke WebView for 2K/daily users for me.

Comment 103 by tobiasjs@chromium.org, Jan 15 2018

This bug was reported at the point that m63 was stable. While I appreciate that this represents an inconvenience for your users, there are a number of points that you should consider:

1) A stable respin carries its own risks, and also a significant cost when you consider the ~1B users who receive a second update.
2) Calling loadUrl from shouldOverrideUrlLoading is not something that we were aware that developers were doing, and thus was not covered by our automated or manual testing.
3) You had a 6 week beta window during which you could have been testing your app and reporting bugs. This issue was fixed within 5 days, and has it been reported in a timely fashion it would have been fixed without reaching stable.

We are definitely aware of the fact that PlzNavigate was a disruptive change. It was also necessary to achieve a number of architectural goals in the wider Chrome project. The WebView team is making changes to our processes in order to decrease the likelihood of this happening and examining the possibility of providing alternative means to achieve the goal of shouldOverrideUrlLoading+loadUrl so that developers don't have to do this.

The comment to which you refer is not immature, and provides the only practical advice given the situation. Please keep your tone civil and constructive in future.

Comment 104 by uzmanbil...@gmail.com, Jan 25 2018

Hi,you must delete the loadUrl method in the shouldOverrideUrlLoading element. Override it. Then it works
Screenshot_1.png
6.7 KB View Download

Comment 105 by alizulfu...@gmail.com, Jan 27 2018

Sorry guys, may i ask question? im also affected one from this bug. im app developer and my apps using webview. should i tell to my users that they should update Chrome browser to latest version? will it be solved if they update Chrome browser to latest version? if itsnot that i must not tell, then what to tell them? what they should do for not affect this bug? Thanks

i use in my app these codes

switch (item.getItemId()) {
            case R.id.back_button:

                if (webView.canGoBack()) {
                    webView.goBack();

                }
                else
                {
                    finish();
                }
        }


and:



private void initWebView() {
webView.setWebViewClient(new WebViewClient() {
@Override
            public boolean shouldOverrideUrlLoading(WebView view, String url) {
                webView.loadUrl(url);
                return true;
            }

            @Override
            public boolean shouldOverrideUrlLoading(WebView view, WebResourceRequest request) {
                if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) {
                    webView.loadUrl(request.getUrl().toString());
                }
                return true;
            }
});

Comment 106 by 78len...@gmail.com, Jan 29 2018

Hi, when is v64 going live? I still dont see v64 in Playstore. Im in Au region

Comment 107 by ntdat1...@gmail.com, Jan 29 2018

Hi, I saw the status is fixed but actually, my device using v63 (android 7) still occur goBack problem. So when the latest version be update to store? Thanks

Comment 108 by arthursonzogni@google.com, Jan 29 2018

Chrome M64 has been released on Android a few days ago:
https://chromereleases.googleblog.com/2018/01/chrome-for-android-update.html

Its availability on the play store is progressive. In the article, it is written:
"(it) will be available on Google Play over the course of the next few weeks"

Comment 109 by ntdat1...@gmail.com, Jan 29 2018

Dear @arthursonzogni, thanks for your information, I think I should wait for a few weeks.

Comment 110 by alizulfu...@gmail.com, Jan 29 2018

in IT has a 1 rule which nobody should forget, "Don't change, don't touch the thing which is working"

Comment 111 by torne@chromium.org, Jan 29 2018

re comment 105: there's no reason to implement shouldOverrideUrlLoading that way - if that's what your implementation of that method does, then it doesn't need to exist at all. :)

shouldOverrideUrlLoading is for when you want the URL load to do something *other* than load the URL in question; if you're just calling loadUrl with the same URL and no other parameters then you are telling to do what it would have done anyway, which slows everything down and means you're affected by this bug when you needn't be.

Comment 112 by alizulfu...@gmail.com, Jan 29 2018

Re comment 111 : thanks for advice and clarification :)

Comment 113 by 78len...@gmail.com, Jan 29 2018

Hi Torne@Chromium.org. Is that a good advice from comment 104. It seems to work. I have yet to test it properly.

Comment 114 by torne@chromium.org, Jan 29 2018

Any override of shouldOverrideUrlLoading should end up doing exactly one of these things for any given call:

1) returning false, without calling loadUrl, to just continue loading as normal
2) returning true, after calling loadUrl with some *different* URL or parameters
3) returning true, after firing off an intent or an in-app navigation or some other outside-of-WebView behaviour
4) returning true after doing nothing else, to simply cancel that load entirely

If you're taking some alternative action to handle the navigation, you should do that and return true. If you just want the navigation to happen as normal, you should do *nothing else other than returning false*.

There's no reason to ever call loadUrl() without changing anything else about the URL/parameters - that just does the same thing as returning false, but worse (slower, and with exposure to this bug).

The case where this bug is actually relevant to a properly-implemented shouldOverrideUrlLoading is the case where the app is calling loadUrl with the same URL, but with *different* header parameters - a number of apps do this to change header values, and that triggers this bug which treats this as a reload incorrectly. If you aren't changing the headers, you shouldn't ever be calling loadUrl with the same URL in the first place, and if you hadn't, you wouldn't have been affected by this bug :)

Comment 115 by alizulfu...@gmail.com, Jan 30 2018

comment 114: If as you said, then for my app itsnot important to use shouldOverrideUrlLoading, then i just deleted it

Comment 116 by ade...@gmail.com, Jan 30 2018

Be careful with "deleting" shouldOverrideUrlLoading, you still have to set
the WebViewClient, read the documentation:
https://developer.android.com/reference/android/webkit/WebViewClient.html#shouldOverrideUrlLoading(android.webkit.WebView,%20java.lang.String)

Comment 117 by alizulfu...@gmail.com, Jan 30 2018

Comment 116: Thanks. I deleted already from all my apps and tested in android OS 5.0, but up of 5.0 i didnt check. I have WebViewClient which inside have onpagefinished and various implementtions. Now im on the way, cant put code here, thanks for advice

Comment 118 by alizulfu...@gmail.com, Jan 30 2018

Sorry, is it safe like this? without shouldOverrideUrlLoading

String current_page_url = "file:///android_asset/web/index.html";

private void initWebView() {
        webView.setWebViewClient(new WebViewClient() {
            @Override
            public void onPageStarted(WebView view, String url, Bitmap favicon) {
                super.onPageStarted(view, url, favicon);
                current_page_url = url;
               
            }

            @Override
            public void onPageFinished(WebView view, String url) {
                super.onPageFinished(view, url);
                
            }

            @Override
            public void onReceivedError(WebView view, WebResourceRequest request, WebResourceError error) {
                super.onReceivedError(view, request, error);
            
            }
        });

        webView.setBackgroundColor(getSelectedItem());
        WebSettings webSettings = webView.getSettings();
        webSettings.setLayoutAlgorithm(WebSettings.LayoutAlgorithm.NARROW_COLUMNS);
        webView.getSettings().setBuiltInZoomControls(true);
        webView.getSettings().setSupportZoom(true);
        webView.getSettings().setDisplayZoomControls(false);
        webView.getSettings().setJavaScriptEnabled(true);
        webView.setWebChromeClient(new WebChromeClient());

    }

Comment 119 by torne@chromium.org, Jan 30 2018

Yes. As long as you have set a WebViewClient, the default behaviour is simply "return false", which will simply load all URLs inside the webview. You only need to implement it if you actually want to override loading to do something else.

Comment 120 by alizulfu...@gmail.com, Jan 30 2018

Thanks again for great advices
Showing comments 21 - 120 of 120 Older

Sign in to add a comment