Flicker bug with preexisting tabs after opening a new tab from intent |
||||||||
Issue descriptionChrome 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.
,
Oct 26 2016
In the example, the cnbc tab is opened in Chrome, the verge tab is opened from a link in another app.
,
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.
,
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.
,
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/
,
Oct 27 2016
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)
,
Nov 3 2016
I also see black flash when I click on a homescreen shortcut, either webapp/fullscreen one, or the normal tab one.
,
Nov 5 2016
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"
,
Nov 5 2016
#7 is flicker, but the original bug is about persistent black in the tab switcher.
,
Nov 5 2016
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.
,
Nov 5 2016
I'll try to get the fix into 55 if it seems safe.
,
Nov 14 2016
Issue 646314 has been merged into this issue.
,
Nov 14 2016
,
Nov 15 2016
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.
,
Nov 15 2016
OK, fix up at https://codereview.chromium.org/2498253002
,
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
,
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
,
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
,
Nov 16 2016
,
Nov 16 2016
Your change meets the bar and is auto-approved for M55 (branch: 2883)
,
Nov 17 2016
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
,
Nov 17 2016
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by aelias@chromium.org
, Oct 26 2016