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

Issue 659459 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
inactive
Closed: Nov 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug

Blocking:
issue 665124



Sign in to add a comment

Flicker bug with preexisting tabs after opening a new tab from intent

Project Member Reported by klo...@chromium.org, Oct 26 2016

Issue description

Chrome Dev 56.0.2900.3.

In the past couple of days, I noticed more black screenshot in the tab switcher in the Chrome Dev on Android. See attached.
 
black.png
197 KB View Download

Comment 1 by aelias@chromium.org, Oct 26 2016

Cc: mdjones@chromium.org
Hmm, I'm trying a bunch of ways of interacting with tabs, but no repro so far.  Do you remember how you created (and moved away from) those tabs?

Comment 2 by klo...@chromium.org, Oct 26 2016

In the example, the cnbc tab is opened in Chrome, the verge tab is opened from a link in another app.

Comment 3 by klo...@chromium.org, Oct 27 2016

I figured out the repro steps.

Make sure that you have two homescreen shortcuts. One is webapp capable, e.g. twitter.com, one is normal, e.g. cnn.com.

1. Open Chrome Dev and make sure you have one tab open.
2. Go to home, tap webapp capable shortcut. It should open a fullscreen activity.
3. Go to home, tap the normal shortcut. It should open in Chrome Dev.
4. Check the tab switcher.

You should get the black tab for the one in 1. 

Comment 4 by aelias@chromium.org, Oct 27 2016

I can repro.  There is another nuance needed to repro.  The normal shortcut tab needs to be a preexisting tab which is reused.  So add step:

0. Go to home, tap the normal shortcut a first time.

Comment 5 by aelias@chromium.org, Oct 27 2016

Note that I always see a white tab, not a black tab.  I can also repro in 54.0.2840.68, probably a fairly longstanding bug.  An example webapp capable shortcut site to repro is http://try.discourse.org/
I am able to repro on Nexus 5X/NRD91N as per #3 (Black tab here on 56.0.2900.3) and #5(white tab here 54.0.2840.68)
I also see black flash when I click on a homescreen shortcut, either webapp/fullscreen one, or the normal tab one.
Summary: Flicker bug with preexisting tabs after opening a new tab from intent (was: see more black screenshot in the Dev tab switcher)
I'm planning to investigate this next week (if I can find the free cycles from release blockers and codereviews).

Copying other repro steps by jayker@ from http://crbug.com/653249.  It's similar enough that I think it might be the same bug.  (If not, I'll triage further.)

"Re-opening since a flash/flicker is still occurring with different repro steps:
1. Open any webpage in a regular Chrome tab.
2. Using the AGSA Google app, search for a query and click on one of the search results to launch a custom tab.
3. Using the custom tabs client example app, "warmup" and "may launch" any url and then launch a custom tab.
4. Switch between the custom tabs or the Chrome tab via Recents.

Observed behavior:
The content in the custom tab flickers.

Expected behavior:
The content in the custom tab should not flash/flicker.

Important Notes:
- This seems to happen more reliably if "warmup" and "may launch" are called before launching the custom tab. For e.g, in repro step 3 above, if the tab is launched without "warmup" and "may launch", I wasn't able to repro the flickering consistently.
- I have added a video showing current behavior here: http://go/chrome-androidlogs1/6/653249 (Ignore the artifacts at the bottom of the video, but that seems to be a problem with screenrecord)

Device: Pixel XL / NMF26D"
#7 is flicker, but the original bug is about persistent black in the tab switcher.
Yeah, I understand.  But I think they are both problems of thumbnail not displayed on preexisting tab, might be fixed by the same change.  It would show up as a flicker in jayker@'s case because it quickly gets replaced by the live tab, but it may be exact same bug.  Anyway, I'll definitely double-check both cases before marking this fixed.
Labels: -M-56 M-55 ReleaseBlock-Stable
I'll try to get the fix into 55 if it seems safe.
Issue 646314 has been merged into this issue.
Blocking: 665124
I think #8 is separate after all, I filed it as  http://crbug.com/665117 .  I also created master bug  http://crbug.com/665124  to keep track of all these.

I investigated this one and the scenario is that at the time the normal shortcut is tapped on, TabModelSelectorImpl calls cacheTabBitmap on the previously "visible" tab.  In TabContentManager::CacheTab(), it tries to readback the latest content from the old tab, but the conditions for readback are not met in this if statement: https://cs.chromium.org/chromium/src/chrome/browser/android/compositor/tab_content_manager.cc?q=tab_content_manager+GetRenderViewHost&sq=package:chromium&l=221&dr=C , and as the metadata has already been clobbered, it removes the thumbnail.  The fix might be to check the readback feasibility earlier so that we can preserve the old thumbnail instead.
Project Member

Comment 16 by bugdroid1@chromium.org, Nov 16 2016

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

commit 834bda0d8d9e1f244539e898b211542ba061db25
Author: aelias <aelias@google.com>
Date: Wed Nov 16 00:42:51 2016

Fix three causes of blank thumbnails.

All three of these fixes are required to address the devil case
 http://crbug.com/659459 , and some of them also help with some of the
other flickers.

1. In onPageLoadStarted, the URL can be the same as before, in
particular because history-item scroll offset restores count as load
starts.  These history restores often fire late in page load, *after*
the thumbnail was taken, leading to immediately throwing out the
thumbnail we just took earlier in page load.  Switch to the "invalidate"
path which only ejects the thumbnail if the URL changed.

2. In CacheTab, which is often called just to refresh the thumbnail with
the latest state, if readback is currently impossible, we used to remove
the old thumbnail.  Keep it instead.

3. Switch to the more reliable Surfaces-based mechanism for readback.
The layer-based mechanism often outputs black screenshots in difficult
racy cases like onStop readbacks.  The Surface-based mechanism had been
reverted because it tickles a driver bug on Nexus 7 2013 that itself causes
some flickers, but I verified that the repro scenario for that is quite
niche, and even on Nexus 7 it's probably less severe than the black
screenshot problem this fixes.

NOTRY=true
BUG= 636630 ,640561, 659459 ,646336

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

[modify] https://crrev.com/834bda0d8d9e1f244539e898b211542ba061db25/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java
[modify] https://crrev.com/834bda0d8d9e1f244539e898b211542ba061db25/chrome/browser/android/compositor/tab_content_manager.cc
[modify] https://crrev.com/834bda0d8d9e1f244539e898b211542ba061db25/ui/android/delegated_frame_host_android.cc

Project Member

Comment 17 by bugdroid1@chromium.org, Nov 16 2016

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

commit 8004ebdebfca6a9d02ebb4d059f3ffc4dafa646f
Author: horo <horo@chromium.org>
Date: Wed Nov 16 01:14:05 2016

Revert of Fix three causes of blank thumbnails. (patchset #1 id:1 of https://codereview.chromium.org/2498253002/ )

Reason for revert:
Caused compile error.
https://build.chromium.org/p/chromium.linux/builders/Cast%20Android%20%28dbg%29/builds/56195

[920/3963] CXX obj/ui/android/android/delegated_frame_host_android.o
FAILED: obj/ui/android/android/delegated_frame_host_android.o
/b/c/cipd/goma/gomacc ../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF obj/ui/android/android/delegated_frame_host_android.o.d -DUI_ANDROID_IMPLEMENTATION -DV8_DEPRECATION_WARNINGS -DENABLE_NOTIFICATIONS -DENABLE_PLUGINS=1 -DUSE_OPENSSL_CERTS=1 -DNO_TCMALLOC -DUSE_EXTERNAL_POPUP_MENU=1 -DENABLE_WEBRTC=1 -DDISABLE_NACL -DUSE_PROPRIETARY_CODECS -DCHROMIUM_BUILD -DFIELDTRIAL_TESTING_ENABLED -DCR_CLANG_REVISION=284979-2 -D_FILE_OFFSET_BITS=64 -DANDROID -DHAVE_SYS_UIO_H -DANDROID_NDK_VERSION=r12b -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__GNU_SOURCE=1 -D__compiler_offsetof=__builtin_offsetof -Dnan=__builtin_nan -D_DEBUG -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DWTF_USE_DYNAMIC_ANNOTATIONS=1 -DUSE_EGL -DDISABLE_FFMPEG_VIDEO_DECODERS -DSK_IGNORE_DW_GRAY_FIX -DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS -DSK_SUPPORT_GPU=1 -DSK_BUILD_FOR_ANDROID -DUSE_CHROMIUM_SKIA -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION -DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE -I../.. -Igen -I../../third_party/khronos -I../../gpu -Igen/ui/android/ui_android_jni_headers -Igen/ui/android/ui_android_jni_headers/ui_android -I../../skia/config -I../../skia/ext -I../../third_party/skia/include/c -I../../third_party/skia/include/config -I../../third_party/skia/include/core -I../../third_party/skia/include/effects -I../../third_party/skia/include/images -I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops -I../../third_party/skia/include/pdf -I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports -I../../third_party/skia/include/utils -I../../third_party/skia/include/gpu -I../../third_party/skia/src/gpu -I../../third_party/skia/src/sksl -I../../third_party/icu/source/common -I../../third_party/icu/source/i18n -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector -funwind-tables -fPIC -pipe -fcolor-diagnostics -ffunction-sections -fno-short-enums --target=arm-linux-androideabi -march=armv7-a -mfloat-abi=softfp -mthumb -mtune=generic-armv7-a -mfpu=neon -Wall -Werror -Wextra -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing -Wno-covered-switch-default -Wno-deprecated-register -Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override -Wno-shift-negative-value -Wno-undefined-var-template -Wno-nonportable-include-path -Wno-address-of-packed-member -Os -fno-omit-frame-pointer -gdwarf-3 -g1 --sysroot=../../third_party/android_tools/ndk/platforms/android-16/arch-arm -fvisibility=hidden -Xclang -load -Xclang ../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so -Xclang -add-plugin -Xclang find-bad-constructs -Wheader-hygiene -Wstring-conversion -fno-threadsafe-statics -fvisibility-inlines-hidden -Wno-undefined-bool-conversion -Wno-tautological-undefined-compare -std=gnu++11 -fno-rtti -isystem../../third_party/android_tools/ndk/sources/cxx-stl/llvm-libc++/libcxx/include -isystem../../third_party/android_tools/ndk/sources/cxx-stl/llvm-libc++abi/libcxxabi/include -isystem../../third_party/android_tools/ndk/sources/android/support/include -fno-exceptions -c ../../ui/android/delegated_frame_host_android.cc -o obj/ui/android/android/delegated_frame_host_android.o
../../ui/android/delegated_frame_host_android.cc:184:42: error: too many arguments to function call, expected single argument 'copy_request', have 2 arguments
                                         std::move(copy_output_request));
                                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../cc/surfaces/surface_factory.h:70:3: note: 'RequestCopyOfSurface' declared here
  void RequestCopyOfSurface(std::unique_ptr<CopyOutputRequest> copy_request);
  ^
1 error generated.

Original issue's description:
> Fix three causes of blank thumbnails.
>
> All three of these fixes are required to address the devil case
>  http://crbug.com/659459 , and some of them also help with some of the
> other flickers.
>
> 1. In onPageLoadStarted, the URL can be the same as before, in
> particular because history-item scroll offset restores count as load
> starts.  These history restores often fire late in page load, *after*
> the thumbnail was taken, leading to immediately throwing out the
> thumbnail we just took earlier in page load.  Switch to the "invalidate"
> path which only ejects the thumbnail if the URL changed.
>
> 2. In CacheTab, which is often called just to refresh the thumbnail with
> the latest state, if readback is currently impossible, we used to remove
> the old thumbnail.  Keep it instead.
>
> 3. Switch to the more reliable Surfaces-based mechanism for readback.
> The layer-based mechanism often outputs black screenshots in difficult
> racy cases like onStop readbacks.  The Surface-based mechanism had been
> reverted because it tickles a driver bug on Nexus 7 2013 that itself causes
> some flickers, but I verified that the repro scenario for that is quite
> niche, and even on Nexus 7 it's probably less severe than the black
> screenshot problem this fixes.
>
> NOTRY=true
> BUG= 636630 ,640561, 659459 ,646336
>
> Committed: https://crrev.com/834bda0d8d9e1f244539e898b211542ba061db25
> Cr-Commit-Position: refs/heads/master@{#432322}

TBR=aelias@chromium.org,tedchoc@chromium.org,aelias@google.com
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 636630 ,640561, 659459 ,646336

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

[modify] https://crrev.com/8004ebdebfca6a9d02ebb4d059f3ffc4dafa646f/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java
[modify] https://crrev.com/8004ebdebfca6a9d02ebb4d059f3ffc4dafa646f/chrome/browser/android/compositor/tab_content_manager.cc
[modify] https://crrev.com/8004ebdebfca6a9d02ebb4d059f3ffc4dafa646f/ui/android/delegated_frame_host_android.cc

Project Member

Comment 18 by bugdroid1@chromium.org, Nov 16 2016

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

commit d9c930e47745312f5cc8a5b9d4e6802c18f8b5de
Author: aelias <aelias@chromium.org>
Date: Wed Nov 16 06:48:22 2016

Fix three causes of blank thumbnails [2nd land]

All three of these fixes are required to address the devil case
 http://crbug.com/659459 , and some of them also help with some of the
other flickers.

1. In onPageLoadStarted, the URL can be the same as before, in
particular because history-item scroll offset restores count as load
starts.  These history restores often fire late in page load, *after*
the thumbnail was taken, leading to immediately throwing out the
thumbnail we just took earlier in page load.  Switch to the "invalidate"
path which only ejects the thumbnail if the URL changed.

2. In CacheTab, which is often called just to refresh the thumbnail with
the latest state, if readback is currently impossible, we used to remove
the old thumbnail.  Keep it instead.

3. Switch to the more reliable Surfaces-based mechanism for readback.
The layer-based mechanism often outputs black screenshots in difficult
racy cases like onStop readbacks.  The Surface-based mechanism had been
reverted because it tickles a driver bug on Nexus 7 2013 that itself causes
some flickers, but I verified that the repro scenario for that is quite
niche, and even on Nexus 7 it's probably less severe than the black
screenshot problem this fixes.

TBR=tedchoc@chromium.org
BUG= 636630 ,640561, 659459 ,646336

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

[modify] https://crrev.com/d9c930e47745312f5cc8a5b9d4e6802c18f8b5de/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java
[modify] https://crrev.com/d9c930e47745312f5cc8a5b9d4e6802c18f8b5de/chrome/browser/android/compositor/tab_content_manager.cc
[modify] https://crrev.com/d9c930e47745312f5cc8a5b9d4e6802c18f8b5de/ui/android/delegated_frame_host_android.cc

Labels: Merge-Request-55

Comment 20 by dimu@chromium.org, Nov 16 2016

Labels: -Merge-Request-55 Merge-Approved-55 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M55 (branch: 2883)
Project Member

Comment 21 by bugdroid1@chromium.org, Nov 17 2016

Labels: -merge-approved-55 merge-merged-2883
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b27a7d7a86cfdc06c04ab3728e08ff10f786b975

commit b27a7d7a86cfdc06c04ab3728e08ff10f786b975
Author: Alexandre Elias <aelias@chromium.org>
Date: Thu Nov 17 02:02:30 2016

Fix three causes of blank thumbnails [2nd land]

All three of these fixes are required to address the devil case
 http://crbug.com/659459 , and some of them also help with some of the
other flickers.

1. In onPageLoadStarted, the URL can be the same as before, in
particular because history-item scroll offset restores count as load
starts.  These history restores often fire late in page load, *after*
the thumbnail was taken, leading to immediately throwing out the
thumbnail we just took earlier in page load.  Switch to the "invalidate"
path which only ejects the thumbnail if the URL changed.

2. In CacheTab, which is often called just to refresh the thumbnail with
the latest state, if readback is currently impossible, we used to remove
the old thumbnail.  Keep it instead.

3. Switch to the more reliable Surfaces-based mechanism for readback.
The layer-based mechanism often outputs black screenshots in difficult
racy cases like onStop readbacks.  The Surface-based mechanism had been
reverted because it tickles a driver bug on Nexus 7 2013 that itself causes
some flickers, but I verified that the repro scenario for that is quite
niche, and even on Nexus 7 it's probably less severe than the black
screenshot problem this fixes.

TBR=tedchoc@chromium.org
BUG= 636630 ,640561, 659459 ,646336

Review URL: https://codereview.chromium.org/2503363003 .

Review-Url: https://codereview.chromium.org/2503183003
Cr-Original-Commit-Position: refs/heads/master@{#432411}
Cr-Commit-Position: refs/branch-heads/2883@{#597}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/b27a7d7a86cfdc06c04ab3728e08ff10f786b975/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java
[modify] https://crrev.com/b27a7d7a86cfdc06c04ab3728e08ff10f786b975/chrome/browser/android/compositor/tab_content_manager.cc
[modify] https://crrev.com/b27a7d7a86cfdc06c04ab3728e08ff10f786b975/ui/android/delegated_frame_host_android.cc

Status: Fixed (was: Assigned)

Sign in to add a comment