Issue metadata
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 descriptionDevice 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.
,
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.
,
Oct 21 2016
Bisection points to CL https://codereview.chromium.org/1364863002
,
Oct 25 2016
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.
,
Nov 4 2016
sorry this fell from my radar. Gustav, do you have time to work on this?
,
Nov 4 2016
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).
,
Nov 4 2016
I got hold of a Samsung Galaxy J1 but it's running KitKat and won't take any OTAs to Lollipop AFAICT >.<
,
Nov 4 2016
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.
,
Nov 7 2016
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?
,
Nov 7 2016
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.
,
Nov 7 2016
Filed b/32698157
,
Nov 12 2016
internal bug b/32629339 is caused by this.
,
Nov 12 2016
,
Nov 21 2016
[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!
,
Nov 22 2016
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?
,
Nov 22 2016
Concur, we can remove RB-S for M55 and retarget for M56.
,
Dec 1 2016
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?
,
Dec 1 2016
,
Dec 10 2016
Alright, I'm seeing the same thing as Tima describes in comment #2 on a Nexus 6, so I'll investigate further.
,
Dec 10 2016
Verified that this will be fixed when b/32629339 is fixed - see my comments #5 and #6 in b/32698157 .
,
Dec 28 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b22a26e4c12a8fc5c22d623e99674335e26cf5e3 commit b22a26e4c12a8fc5c22d623e99674335e26cf5e3 Author: japhet <japhet@chromium.org> Date: Wed Dec 28 17:48:37 2016 Always send a fail or finish notification for each navigation. Historically, we have dropped these when navigations are interleaved. BUG= 656919 Review-Url: https://codereview.chromium.org/2563423004 Cr-Commit-Position: refs/heads/master@{#440862} [modify] https://crrev.com/b22a26e4c12a8fc5c22d623e99674335e26cf5e3/third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload-expected.txt [modify] https://crrev.com/b22a26e4c12a8fc5c22d623e99674335e26cf5e3/third_party/WebKit/Source/core/loader/DocumentLoader.cpp [modify] https://crrev.com/b22a26e4c12a8fc5c22d623e99674335e26cf5e3/third_party/WebKit/Source/core/loader/DocumentLoader.h [modify] https://crrev.com/b22a26e4c12a8fc5c22d623e99674335e26cf5e3/third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp [modify] https://crrev.com/b22a26e4c12a8fc5c22d623e99674335e26cf5e3/third_party/WebKit/Source/core/loader/FrameLoader.cpp [modify] https://crrev.com/b22a26e4c12a8fc5c22d623e99674335e26cf5e3/third_party/WebKit/Source/modules/webaudio/BaseAudioContextTest.cpp
,
Jan 4 2017
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).
,
Jan 4 2017
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.
,
Jan 4 2017
Right, this bug in particular doesn't seem to be of super-higher prio :), does your comment hold for b/32629339 as well, Alex?
,
Jan 9 2017
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.
,
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
,
Jan 10 2017
The CL in comment #26 fixes this for 57 (56 is still buggy).
,
Jan 11 2017
Potential fix for 56 here: https://codereview.chromium.org/2622283002
,
Jan 12 2017
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
,
Jan 12 2017
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
,
Jan 12 2017
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
,
Jan 12 2017
,
Jan 17 2017
Issue Doesn't repro on latest M57 build as per comment #0. Thanks
,
Jan 18 2017
don't see this issue on latest M56 webview on Galaxy S7 ATT edge
,
Jan 18 2017
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 |
|||||||||||||||||||||||
Comment 1 by acindhe@chromium.org
, Oct 18 2016Status: Available (was: Unconfirmed)