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

Issue 656919 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Last visit > 30 days ago
Closed: Jan 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Flash of contents is seen on header part of spreadsheet while going fullscreen in "Google Drive".

Reported by pshi...@etouch.net, Oct 18 2016

Issue description

Device name:Micromax Q372/LRX21M,Gionee F103/LRX21M,Samsung Galaxy J5/MMB29M,HTC Desire 630/MMB29M,Asus Zenfone Max/MMB29P
WebView version: 56.0.2894.0,55.0.2883.18,54.0.2840.67
Application: Google Drive	
Application version: 2.4.382.16.44

Bisect Range: https://chromium.googlesource.com/chromium/src/+log/54.0.2809.0..54.0.2810.0?pretty=fuller&n=10000

Pre-condition: Make sure 'Google Sheets' is not installed on the device.

Steps to reproduce:
(1)Launch google drive,open any spreadsheet 
(2)Tap thrice or multiple times in the middle of screen
(3)Observe

Frequency: 3/3

Expected result: The spreadsheet should load smoothly while going fullscreen.

Actual result: Flash of contents is seen on header part of spreadsheet while going fullscreen.

 
Labels: -Pri-3 M-55 ReleaseBlock-Stable Pri-1 Type-Bug-Regression
Status: Available (was: Unconfirmed)
This is a regression. Added Release block stable label , please feel free to remove release blocker label if not a blocker. Thanks 

Please find the logcat & video @ http://go/chrome-androidlogs1/6/656919

Comment 2 by ti...@chromium.org, Oct 20 2016

Tested with Nexus 6P, Android N (angler-userdebug 7.0 NRD90 3020500 dev-keys).

I cannot really see the "flash of contents" from the original description, but I noticed that in the bad case (54.0.2810.0) the system buttons in the bottom disappear and reappear very quickly, and then fade away. In the good case they just fade away after a small pause.

Attaching recorded screen captures.

sysbuttons-normal.mp4
1.2 MB View Download
sysbuttons-jump.mp4
1.4 MB View Download

Comment 3 by ti...@chromium.org, Oct 21 2016

Cc: sgu...@chromium.org gsennton@chromium.org
Owner: japhet@chromium.org
Bisection points to CL https://codereview.chromium.org/1364863002

Comment 4 by japhet@chromium.org, Oct 25 2016

Cc: japhet@chromium.org
Owner: sgu...@chromium.org
I don't have a rooted phone set up to install a custom webview, and I'm going out on paternity leave any day now. :/

sgurun, is there any chance you'd be willing to pick this up? (Feel free to reassign, to me or otherwise)

I can't imagine the blink changes in https://codereview.chromium.org/1364863002 would cause this behavior, so the 1-line change in AwContents.java would be my guess.
Owner: gsennton@chromium.org
sorry this fell from my radar. Gustav, do you have time to work on this?
Oh gawd, onPageFinished, I can take a look.

I don't think this should be blocking 55 though, given that it is a regression between 53 and 54?


I tried this on a Nexus 6 running LMY49S with Google Drive 2.4.382.19.30 (I don't know how to get the version of Drive used by the reporter) and WebView 54.0.2840.67, and I can't reproduce the flickering.

A also tried this on a Nexus 6 running N (NRD88) with Google Drive 2.4.382.19.30 and WebView 54.0.2840.67 and I can't repro then either.

We don't seem to have any of the devices from the original report in the London skylab, I'll try to see if I can get my hands on a Samsung Galaxy J1 (not a J5).
I got hold of a Samsung Galaxy J1 but it's running KitKat and won't take any OTAs to Lollipop AFAICT >.<
If we're triggering an extra onPageFinished, then the chances are that drive just uses onPageFinished to trigger the the animation to fullscreen, and now that happens twice (whether because there are two onPageFinished calls, or because there is also a different trigger path is unclear).

You probably won't see it on a faster device, for that reason.

It might also explain the initial report; we render some fullscreen content, go back to some non-fullscreen state, and then do it all again.
Right, that makes sense.
I'll try to repro with a nexus 5 later today, otherwise it feels like the easiest way to move this forward would be to ask someone from the Drive team / take a look at their source code?
I'm not seeing this on Nexus 5 either, but I'll try to find someone acquainted with the Drive code that uses WebView to load spreadsheets.
Filed b/32698157
internal bug b/32629339 is caused by this.
Labels: -Restrict-View-Google
[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!
Cc: amineer@chromium.org
I'm still not sure this should block 55, I mean we shipped this bug in 54 - and any changes to callbacks like onReceiveError, onPageFinished and similar have proven be very difficult to get right (and can easily break other apps).

WDYT?
Labels: -M-55 -ReleaseBlock-Stable M-56
Concur, we can remove RB-S for M55 and retarget for M56.
I've tried reproducing this on HTC Desire 530 and I maybe reproduced it.. (it's difficult to tell when toggling fullscreen on and off), anyway, I wanted to install a locally built WebView to debug it and realized I can't really do that on a production phone.

Does this reproduce on any Nexus devices where we can install locally built WebViews?
Labels: Needs-Feedback
Alright, I'm seeing the same thing as Tima describes in comment #2 on a Nexus 6, so I'll investigate further.
Verified that this will be fixed when b/32629339 is fixed - see my comments #5 and #6 in b/32698157 .
Labels: -Needs-Feedback
Alright, this will be fixed (for 57) when we've removed postOnPageFinished from AwContents.shouldInterceptRequest.
(I'll add a test in that CL to ensure we always post onPageFinished even when receiving empty responses).

For 56 we'll have to do something else as well (cherry-picking the CL in #21 would be scary).
FWIW since this has been broken for ~10 weeks, I don't know that I'd prioritize fixing this in M56 over other upcoming Android priorities (wink wink nudge nudge).  If it's easy to create a version of c#21 that's simple and safe then give it a go, but if it's going to take a lot of time then I'm happy to reconsider.
Right, this bug in particular doesn't seem to be of super-higher prio :), does your comment hold for b/32629339 as well, Alex?
Blah, forgot this bug was about more than simply the "white flash" of content as the subject suggests.  The same regression in b/32629339 has been present since M54, right?

If so, I think we have some leeway - if the merge fix would be difficult and / or risky, ping me and we can discuss punting to M57.  If we can do this simply, we should get it fixed.
Project Member

Comment 26 by bugdroid1@chromium.org, Jan 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7363ea0b81a3a51eb1513761ecfee892f667fa64

commit 7363ea0b81a3a51eb1513761ecfee892f667fa64
Author: gsennton <gsennton@chromium.org>
Date: Tue Jan 10 17:29:31 2017

[Android WebView] Remove postOnPageFinished from shouldInterceptRequest.

We previously added a call to onPageFinished from shouldInterceptRequest
in https://codereview.chromium.org/1364863002/.
This was to avoid failing the test
AwContentsClientShouldInterceptRequestTest#testOnReceivedErrorCallback.
However, the cause of the test failing was that with that CL we no
longer post onPageFinished after receiving a response with an empty data
segment (because WebView no longer received
WebContentsObserver.didFinishLoad() callbacks for loads that resulted in
an empty response).
Since then, onPageFinished is again being posted after receiving an
empty response - this was fixed in
https://codereview.chromium.org/2563423004/

The current CL simply removes the unnecessary (and thus incorrect)
onPageFinished call from shouldInterceptRequest and adds a couple of
tests to ensure the original problem with onPageFinished doesn't occur
again.

See b/32629339 for additional discussions.

BUG= 656919 

Review-Url: https://codereview.chromium.org/2624443002
Cr-Commit-Position: refs/heads/master@{#442621}

[modify] https://crrev.com/7363ea0b81a3a51eb1513761ecfee892f667fa64/android_webview/java/src/org/chromium/android_webview/AwContents.java
[modify] https://crrev.com/7363ea0b81a3a51eb1513761ecfee892f667fa64/android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java
[modify] https://crrev.com/7363ea0b81a3a51eb1513761ecfee892f667fa64/net/test/android/javatests/src/org/chromium/net/test/util/TestWebServer.java

The CL in comment #26 fixes this for 57 (56 is still buggy).
Potential fix for 56 here:
https://codereview.chromium.org/2622283002
Labels: Merge-Request-56
Using this bug for the merge-request for https://codereview.chromium.org/2622283002/

Note that the primary bug we are fixing here is b/32629339
Project Member

Comment 30 by sheriffbot@chromium.org, Jan 12 2017

Labels: -Merge-Request-56 Hotlist-Merge-Approved Merge-Approved-56
Your change meets the bar and is auto-approved for M56. Please go ahead and merge the CL manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), gkihumba@(cros), bustamante@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 31 by bugdroid1@chromium.org, Jan 12 2017

Labels: -merge-approved-56 merge-merged-2924
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/00b73e7d682c407bf15fc79e667c75fc3112ebd3

commit 00b73e7d682c407bf15fc79e667c75fc3112ebd3
Author: gsennton <gsennton@chromium.org>
Date: Thu Jan 12 22:19:26 2017

[WebView 56] Post onPageFinished for committed Url if nav to error page.

Problem:
Receiving an empty response for a request results in bad
behaviour w.r.t. WebContentsObserver in 56 (and, 54 and 55). We receive
no didFinishLoad and no didFailLoad. Arguably, this is because the load didn't finish
correctly (empty response -> no didFinishLoad), but the load didn't
quite fail either (the response was returned to the client) -> no
didFailLoad. WebView uses primarily didFinishLoad and didFailLoad to
post onPageFinished calls to the embedding WebView app.
Thus, in the affected versions, WebView wouldn't post onPageFinished
when receiving and empty response for a request.

Short-term solution [56]
Currently, we post onPageFinished from didStopLoading (all navigations
have finished) for the current Url only if that was the last Url for
which we received didFinishLoad. With this CL, also post onPageFinished
(from didStopLoading) if the current Url was committed when we started
loading an error page. This catches the empty-response case since in
that case the Url for which we receive no didFinishLoad/didFailLoad
first commits and then an error page is loaded.

With this CL also remove an unnecessary posting of onPageFinished from
shouldInterceptRequest - this change is the same as the one in
https://codereview.chromium.org/2624443002

Long-term solution [57+]
The issue at hand was fixed on master in
b22a26e4c12a8fc5c22d623e99674335e26cf5e3
https://codereview.chromium.org/2563423004/
but we didn't want to merge that change to 56.

BUG=b/32629339
BUG= 656919 

NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2622283002
Cr-Commit-Position: refs/branch-heads/2924@{#747}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/00b73e7d682c407bf15fc79e667c75fc3112ebd3/android_webview/java/src/org/chromium/android_webview/AwContents.java
[modify] https://crrev.com/00b73e7d682c407bf15fc79e667c75fc3112ebd3/android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java

Status: Fixed (was: Available)
Issue Doesn't repro on latest M57 build as per comment #0. Thanks 
don't see this issue on latest M56 webview on Galaxy S7 ATT edge
Status: Verified (was: Fixed)
Issue doesn't repro on latest M57 & M56 builds. Hence closing this issue. Thanks 

Devices tested:
Micromax Q372/LRX21M,
Gionee F103/LRX21M,
Samsung Galaxy J5/MMB29M,
HTC Desire 630/MMB29M,
HTC One M8 / MRA58K 

Sign in to add a comment