Could you try re-install the WebAPK to see if the bug still exists?
I can reproduce the issue yesterday but now seems it's being resolved.

Still reproduces on 69.0.3486.0 with a fresh install of the app.

I and Xi are trying to reproduce the bug. We found it's only reproducible in one personal Pixel 2 device, and it's only reproducible in Chrome Canary/Dev but not in tot.

Steps trying to reproduce using tot:
1. Install webapk using Chrome Canary
2. Uninstall Chrome Canary
3. Build and install Clankium using tot
4. Open webapk, in the select host browser dialog, select clankium

Is there anything we are missing? Any idea why it's not reproducible using tot?

So it's not reproducible on other Pixel 2s?

Not reproducible on one Pixel device and one Pixel XL device both running android 8.1. Also not reproducible in any device lower than Android O.

Can't reproduce using tot is wried, because tot should always be newer than Canary.

We can reproduce on ToT if we build release official.
It suggets it's probably a race condition. Further example is that on debug build you can see the top controls for a brief flash before it disappears entirely.

+tedchoc +jinsukkim (who made some changes to ViewANdroidDelegate.onTopControlsChanges at one point)

+fsamuel@ who looked into the browser controls a few months ago

Bisect based on Yaron's observation (a brief flash before disappearing) points to https://chromium.googlesource.com/chromium/src/+/0ec77df64a7dbf67bbf53885b0dfa8b6ed16450d 

Assigning to the owner of the CL.

piotrs no longer works on chromium. Reassigning to ranj for now, but that might change pending whether recent cc's find a race condition here.

Furthermore, piotr's CL only applies the transient behaviour to non-WebAPKs. Is it possible that this has happened:

1. WebAPKs didn't use the codepath introduced by piotrs@ in https://chromium.googlesource.com/chromium/src/+/0ec77df64a7dbf67bbf53885b0dfa8b6ed16450d because they opened a fresh CCT

2. ranj's fixes for  Issue 830946  and  Issue 831806  routed WebAPKs back into using the non-WebAPK codepath, which is causing the race condition (transiently showing the browser controls, but interfering with the hiding in the WebAPK code path).

Without digging into the code, that matches my assumptions. I'm guessing we may have always had this problem for webapps or it regressed after impl and webapks picked up the regression after webapks switched to using the minimal-ui presentation for out of scope navigations. 

I think it'd be good to confirm whether this behaviour repros outside of webapk and in traditional webapp mode.

I'm unable to repro this locally fwiw.

+pkotwicz
As I'm leaving, mind taking a look at this bug? It can't be reproduced in debug build and my desktop was freezing every time I tried do build official:(

I guess it should have something to do with ExternalNavigation*.

fsamuel@: I can no longer repro the bug as described here. I can however get incorrect UI to show intermittently on Chrome Canary.

https://drive.google.com/file/d/1pOdHFNIL3-Io2MkenEx9UU3VXwN0wrvn/view?usp=sharing

The glitch is visible at the 34 and the 45 second mark

Bug mentioned in comment #16:
- Only reproes if you quickly navigate back after opening the out-of-scope page.
- Chrome spams Tab#updateBrowserControlsState(). The bug reproes with this change as well:
  Tab#updateBrowserControlsState {
    ...
    if (constraints == mBrowserConstrolsConstraints) return;
    nativeUpdateBrowserControlsState(mNativeTabAndroid, constraints, current, animate);
    ...
  }

- I noticed that when the bug occurs:
  WebViewImpl::ApplyViewportDeltas() calls BrowserControls::SetShownRatio() with a fractional value after
  BrowserControls::SetShownRatio(0) is called as a result of BrowserControls::UpdateConstraintsAndState()

Toolbar stays visible regardless of what the user does afterwards:
- I have not been able to get this repro reliably. I have gotten this to repro by adding LOG() statements in the cc code so I suspect that this occurs as a result of a race in the cc code.

Assigning to bokan@ who seems to be most relevant person

Just to update - I've gotten as far as reproducing but have more urgent issues this week so likely wont get to it until next.

Ok - did some investigation here. This is a race between the main and impl threads and committing top controls delta.

An animated UpdateBrowserControlsState is ignored on main and starts animating on impl. A non-animated UBCS clamps to constraints on main immediately and waits on impl until the values propagate through a normal commit cycle.

Unfortunately, as mentioned in #17, we seem to spam UBCS calls and they mix animated and non-animated calls. So we end up clamping on main using a non-animated UBCS and before the clamp is propagated to the impl thread, we also start an animated UBCS. Unfortunately, because clamping doesn't work well across compositor/Blink commits (see  bug 754346  for more details) we get the bug in the report.

I have an idea on how to fix the commit issue in this case (just let the main thread be authoritative and clobber the impl value). I'd probably make sense to also look into why we're sending so many UBCS messages and why they're a mix of animated/non-animated but I'm not sure if I'll have time for that.

Showing browser controls is usually animated.
Hiding browser controls is usually not animated.
(See Tab#updateFullscreenEnabledState())

bokan@: Any updates?

No sorry - I believe I understand the problem now but the issue doesn't seem severe to me so is low on my task list, likely wont get to it this week.

My fix above broke URL bar hiding for scrolling and I haven't had time to investigate if that was a small bug or a problem with my approach.

This is a poor UX - chrome appears broken as a result because the toolbar doesn't actually do anything, and the page is also vertically misaligned. Using minimal UI (the buggy code in question) was also a highly requested feature (issue 771418) in response to folks complaining that authentication in webapks was entirely broken so we know there's definite developer demand for this scenario. Please consider this a pretty please +1 to prioritize this :)

The repro in the original bug doesn't occur for me (nor for pkotowicz@, see #16). So the bug that I've been investigating is a transient freezing of the toolbar - see the video in #16 for what I mean. It takes quite a bit of back and forth to get it to appear, and when it does occur it's transient (goes away in about a second or two) so it seems more in the genre of "visual glitch" than an actual UX issue (which we should still fix, but lower pri). Is the original bug still reproing for others? Let me know if I'm missing something.

I think that this is higher priority. This bug is one that's been popping up every few months in relation to twitter - the #10 most popular site on the internet. (Though I will grant that WebAPKs are not a particularly popular feature).
Here's past bugs:
-  http://crbug.com/827028 
- http://b/77772021

I hope that fixing the bug about the transient glitch will also fix the "twitter bug" that people have reported.

At the time that the "twitter bug" was reported I was unable to repro the bug locally as well.
I know that things have changed since the "twitter bug" was reported. I think that Fady rewrote a lot of the content side plumbing. I *think* that this bug was filed post Fady's changes

Both those bugs seem to be about the top bar being consistently shown. The issue as described in #16 is a transient sync issue in the compositor so I really don't think fixing that will help the twitter bugs if they're still occurring. I'll set aside some time next week to land a fix but I don't think that'll help the issue y'all really want fixed (and that would likely be a problem higher up in the stack rather than in Blink/Compositor). 

bokan@: Any updates?

Yes, I've fixed my patch but I'm now fighting with tests. Actively working on this.

Update - I have a patch in review but am away until Oct 2, will pick it up there

bokan@: Any updates?

Yup, patch is up but it's grown a bit and I've been splitting it up - landed the refactoring. I'm hoping to have the fix landed this week. Follow at: https://chromium-review.googlesource.com/c/chromium/src/+/1205797

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

commit eaa5cc8d062b3ab4152942bd6cc1084cb48c9147
Author: David Bokan <bokan@chromium.org>
Date: Fri Oct 12 15:50:40 2018

Allow SimTests to test BrowserControl animations

This patch adds methods to SimCompositor to allow ticking some basic
impl-side animations. In particular, this is used in
https://crrev.com/c/1205797 to play a browser controls animation from
impl-side code.

Despite the fact that SimCompositor supports only single-threaded mode,
an impl animation can still be initiated. Though there is no "impl
thread" on which the animation can be serviced, we can tick it by
producing main frames using BeginFrame (the commit calls
LayerTreeHostImpl::Animate). BeginFrame checks that a new frame is
actually needed so this adds a plumbing to make this check work with
changes coming from the "impl-side".

Another wrinkle is that the browser controls animation was based on
TimeTicks::Now; the start and end times are set from Now() and the
incoming frame_time is used to interpolate a value between the two. This
CL updates BrowserControlsOffsetManager to get the start time from the
first frame time, allowing tests to use a mock clock. This works more
like scrollbar and other animations but requires fixing tests that
assumed the first tick would produce delta.

We also add plumbing to pass viewport changes to Blink to allow the impl
side changes to propagate to Blink through ApplyViewportChanges, since
things can now change outside of the "main thread" side.

The ScrollSnapTest change was necessary in a previous iteration of this
CL when I made a change to the last_frame_time in SimCompositor. Though
that change was undone in favor of fixing the animation start time in
BrowserControlsOffsetManager, the changes in ScrollSnapTest are still
desirable since they make dispatched events use timestamps that
explicitly correspond to the frame times we use in BeginFrame.

Bug:  861618 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I069268d1b3ae25003f62f3e9b9c2ad54245e402c
Reviewed-on: https://chromium-review.googlesource.com/c/1270344
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599232}
[modify] https://crrev.com/eaa5cc8d062b3ab4152942bd6cc1084cb48c9147/cc/input/browser_controls_offset_manager.cc
[modify] https://crrev.com/eaa5cc8d062b3ab4152942bd6cc1084cb48c9147/cc/input/browser_controls_offset_manager.h
[modify] https://crrev.com/eaa5cc8d062b3ab4152942bd6cc1084cb48c9147/cc/input/browser_controls_offset_manager_unittest.cc
[modify] https://crrev.com/eaa5cc8d062b3ab4152942bd6cc1084cb48c9147/cc/trees/layer_tree_host_impl_unittest.cc
[modify] https://crrev.com/eaa5cc8d062b3ab4152942bd6cc1084cb48c9147/cc/trees/layer_tree_host_unittest_proxy.cc
[modify] https://crrev.com/eaa5cc8d062b3ab4152942bd6cc1084cb48c9147/cc/trees/single_thread_proxy.cc
[modify] https://crrev.com/eaa5cc8d062b3ab4152942bd6cc1084cb48c9147/cc/trees/single_thread_proxy.h
[modify] https://crrev.com/eaa5cc8d062b3ab4152942bd6cc1084cb48c9147/third_party/blink/renderer/core/input/scroll_snap_test.cc
[modify] https://crrev.com/eaa5cc8d062b3ab4152942bd6cc1084cb48c9147/third_party/blink/renderer/core/testing/sim/sim_compositor.cc
[modify] https://crrev.com/eaa5cc8d062b3ab4152942bd6cc1084cb48c9147/third_party/blink/renderer/core/testing/sim/sim_compositor.h

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

commit 2bd206ee01da75d63c8b6e791fc3d8c9786e40bc
Author: David Bokan <bokan@chromium.org>
Date: Sat Oct 13 21:58:24 2018

BrowserControlsState updates CC and commits to Blink

The linked bug is a result of the fact that we send and handle browser
controls state updates on both the main and compositor threads. When a
non-animated hide is followed by a animated hide, we start an animation
on the compositor thread but immediately hide the top controls on the
main thread. When we commit the browser controls delta, the controls
hide but the animation continues to play so the next frame re-shows the
controls. Additional updates may stop the animation so we'd see the
controls in a half-way hidden state.

Currently, UpdateBrowserControlsState sends animated commands to be
handled by CC (since Blink doesn't know how to animate), but because of
 https://crbug.com/754346 , non-animated commands are sent to Blink. When
the browser process sends a series of interweaved commands to the render
this leads the the clamping bug discussed above.

This CL fixes this issue by making the compositor-side the real source-
of-truth and pushing the changes to Blink. Both CC and Blink must be
made aware of the browser controls constraints, since scrolls may be
handled by either. However, we now always inform CC first and let it
push the differences to Blink. This happens as part of
ApplyViewportChanges during a commit.

Bug:  861618 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I6cb6a6480804fff05e6aee65166bf0d303fb677d
Reviewed-on: https://chromium-review.googlesource.com/c/1205797
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: enne <enne@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599523}
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/cc/input/browser_controls_offset_manager.cc
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/cc/input/browser_controls_offset_manager.h
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/cc/input/browser_controls_offset_manager_client.h
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/cc/input/browser_controls_offset_manager_unittest.cc
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/cc/input/browser_controls_state.h
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/cc/trees/layer_tree_host.cc
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/cc/trees/layer_tree_host.h
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/cc/trees/layer_tree_host_client.h
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/cc/trees/layer_tree_host_common.cc
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/cc/trees/layer_tree_host_common.h
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/cc/trees/layer_tree_host_impl.h
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/cc/trees/layer_tree_host_unittest.cc
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/cc/trees/single_thread_proxy.cc
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/cc/trees/single_thread_proxy.h
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/content/public/renderer/render_view.h
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/content/renderer/gpu/gpu_benchmarking_extension.cc
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/content/renderer/render_view_impl.cc
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/content/renderer/render_view_impl.h
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/content/shell/renderer/layout_test/blink_test_runner.cc
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/third_party/blink/public/web/web_view.h
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/third_party/blink/public/web/web_widget.h
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/third_party/blink/renderer/core/exported/web_frame_test.cc
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/third_party/blink/renderer/core/exported/web_view_impl.cc
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/third_party/blink/renderer/core/exported/web_view_impl.h
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/third_party/blink/renderer/core/frame/browser_controls.cc
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/third_party/blink/renderer/core/frame/browser_controls_test.cc
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/third_party/blink/renderer/core/frame/visual_viewport_test.cc
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/third_party/blink/renderer/core/frame/web_view_frame_widget.cc
[modify] https://crrev.com/2bd206ee01da75d63c8b6e791fc3d8c9786e40bc/third_party/blink/renderer/core/frame/web_view_frame_widget.h

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

commit 1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258
Author: Patti <patricialor@chromium.org>
Date: Mon Oct 15 02:39:32 2018

Revert "BrowserControlsState updates CC and commits to Blink"

This reverts commit 2bd206ee01da75d63c8b6e791fc3d8c9786e40bc.

Reason for revert: This patch may be the culprit behind cc_unittest TreeSynchronizerTest.SynchronizeScrollTreeScrollOffsetMap failing on Linux ChromiumOS MSan Tests [20 out of the last 20 builds have failed] and Linux MSan Tests [21 out of the last 21 builds have failed]. Reverting to try and fix.

Original change's description:
> BrowserControlsState updates CC and commits to Blink
> 
> The linked bug is a result of the fact that we send and handle browser
> controls state updates on both the main and compositor threads. When a
> non-animated hide is followed by a animated hide, we start an animation
> on the compositor thread but immediately hide the top controls on the
> main thread. When we commit the browser controls delta, the controls
> hide but the animation continues to play so the next frame re-shows the
> controls. Additional updates may stop the animation so we'd see the
> controls in a half-way hidden state.
> 
> Currently, UpdateBrowserControlsState sends animated commands to be
> handled by CC (since Blink doesn't know how to animate), but because of
>  https://crbug.com/754346 , non-animated commands are sent to Blink. When
> the browser process sends a series of interweaved commands to the render
> this leads the the clamping bug discussed above.
> 
> This CL fixes this issue by making the compositor-side the real source-
> of-truth and pushing the changes to Blink. Both CC and Blink must be
> made aware of the browser controls constraints, since scrolls may be
> handled by either. However, we now always inform CC first and let it
> push the differences to Blink. This happens as part of
> ApplyViewportChanges during a commit.
> 
> Bug:  861618 
> Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
> Change-Id: I6cb6a6480804fff05e6aee65166bf0d303fb677d
> Reviewed-on: https://chromium-review.googlesource.com/c/1205797
> Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
> Reviewed-by: enne <enne@chromium.org>
> Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
> Commit-Queue: David Bokan <bokan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#599523}

TBR=bokan@chromium.org,chrishtr@chromium.org,enne@chromium.org,alexmos@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  861618 
Change-Id: I6738ee1fb616e2b06b5a466546a8f47bcaeb7d79
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Reviewed-on: https://chromium-review.googlesource.com/c/1278460
Reviewed-by: Patti <patricialor@chromium.org>
Commit-Queue: Patti <patricialor@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599556}
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/cc/input/browser_controls_offset_manager.cc
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/cc/input/browser_controls_offset_manager.h
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/cc/input/browser_controls_offset_manager_client.h
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/cc/input/browser_controls_offset_manager_unittest.cc
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/cc/input/browser_controls_state.h
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/cc/trees/layer_tree_host.cc
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/cc/trees/layer_tree_host.h
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/cc/trees/layer_tree_host_client.h
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/cc/trees/layer_tree_host_common.cc
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/cc/trees/layer_tree_host_common.h
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/cc/trees/layer_tree_host_impl.h
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/cc/trees/layer_tree_host_unittest.cc
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/cc/trees/single_thread_proxy.cc
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/cc/trees/single_thread_proxy.h
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/content/public/renderer/render_view.h
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/content/renderer/gpu/gpu_benchmarking_extension.cc
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/content/renderer/render_view_impl.cc
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/content/renderer/render_view_impl.h
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/content/shell/renderer/layout_test/blink_test_runner.cc
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/third_party/blink/public/web/web_view.h
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/third_party/blink/public/web/web_widget.h
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/third_party/blink/renderer/core/exported/web_frame_test.cc
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/third_party/blink/renderer/core/exported/web_view_impl.cc
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/third_party/blink/renderer/core/exported/web_view_impl.h
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/third_party/blink/renderer/core/frame/browser_controls.cc
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/third_party/blink/renderer/core/frame/browser_controls_test.cc
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/third_party/blink/renderer/core/frame/visual_viewport_test.cc
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/third_party/blink/renderer/core/frame/web_view_frame_widget.cc
[modify] https://crrev.com/1c0981de74e8c5cf9aa5fbb1c932aca4b94bd258/third_party/blink/renderer/core/frame/web_view_frame_widget.h

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

commit 1e37ebf04121920e20cdf864584121a388533294
Author: David Bokan <bokan@chromium.org>
Date: Tue Oct 16 13:53:37 2018

[Reland] BrowserControlsState updates CC and commits to Blink

The linked bug is a result of the fact that we send and handle browser
controls state updates on both the main and compositor threads. When a
non-animated hide is followed by a animated hide, we start an animation
on the compositor thread but immediately hide the top controls on the
main thread. When we commit the browser controls delta, the controls
hide but the animation continues to play so the next frame re-shows the
controls. Additional updates may stop the animation so we'd see the
controls in a half-way hidden state.

Currently, UpdateBrowserControlsState sends animated commands to be
handled by CC (since Blink doesn't know how to animate), but because of
 https://crbug.com/754346 , non-animated commands are sent to Blink. When
the browser process sends a series of interweaved commands to the render
this leads the the clamping bug discussed above.

This CL fixes this issue by making the compositor-side the real source-
of-truth and pushing the changes to Blink. Both CC and Blink must be
made aware of the browser controls constraints, since scrolls may be
handled by either. However, we now always inform CC first and let it
push the differences to Blink. This happens as part of
ApplyViewportChanges during a commit.

Reland Note: The original patch broke MSAN as it forgot to initialize
ScrollAndScaleSet::browser_controls_constraint_changed. This CL fixes
that issue.

TBR=enne@chromium.org,chrishtr@chromium.org,alexmos@chromium.org

Bug:  861618 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I77964a893a9e376ceb29a69c6bef8b699801ed82
Reviewed-on: https://chromium-review.googlesource.com/c/1283249
Reviewed-by: David Bokan <bokan@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599963}
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/cc/input/browser_controls_offset_manager.cc
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/cc/input/browser_controls_offset_manager.h
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/cc/input/browser_controls_offset_manager_client.h
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/cc/input/browser_controls_offset_manager_unittest.cc
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/cc/input/browser_controls_state.h
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/cc/trees/layer_tree_host.cc
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/cc/trees/layer_tree_host.h
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/cc/trees/layer_tree_host_client.h
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/cc/trees/layer_tree_host_common.cc
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/cc/trees/layer_tree_host_common.h
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/cc/trees/layer_tree_host_impl.h
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/cc/trees/layer_tree_host_unittest.cc
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/cc/trees/single_thread_proxy.cc
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/cc/trees/single_thread_proxy.h
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/content/public/renderer/render_view.h
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/content/renderer/gpu/gpu_benchmarking_extension.cc
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/content/renderer/render_view_impl.cc
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/content/renderer/render_view_impl.h
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/content/shell/renderer/layout_test/blink_test_runner.cc
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/third_party/blink/public/web/web_view.h
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/third_party/blink/public/web/web_widget.h
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/third_party/blink/renderer/core/exported/web_frame_test.cc
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/third_party/blink/renderer/core/exported/web_view_impl.cc
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/third_party/blink/renderer/core/exported/web_view_impl.h
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/third_party/blink/renderer/core/frame/browser_controls.cc
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/third_party/blink/renderer/core/frame/browser_controls_test.cc
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/third_party/blink/renderer/core/frame/visual_viewport_test.cc
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/third_party/blink/renderer/core/frame/web_view_frame_widget.cc
[modify] https://crrev.com/1e37ebf04121920e20cdf864584121a388533294/third_party/blink/renderer/core/frame/web_view_frame_widget.h

The bug shown in #16 should be fixed in the next Canary - hopefully other related issues too (this turned out be a fairly significant rearranging of how top controls work). It just missed the M71 cut and it's fairly large and spread across the patches in #36 and #33 so my instinct is to avoid a merge here but let me know if you think the issues are serious enough to warrant the merge risk.

Reopening. I tested today with both today's Canary 72.0.3583.0 and a ToT official build (600535)
The top controls still incorrectly show, except that now they stay incorrectly shown. This incorrect behavior now seems to occur close to 100% of the time

Here - https://drive.google.com/file/d/1OThZ5OY_AuEh13AeBYQ6tg6OUaCZigUc/view?usp=sharing is a video with today's Canary

Drats - sorry. Will take another look today.

For the sake of completeness the buggy behavior occurs with a release official build but not a debug build

Strange - I can't repro the issue using Canary on:

N6 - 72.0.3585.0
P2XL - 72.0.3584.0

What device do you see this on?

I saw this on a Pixel 2 running Android P. Just reproed this today with today's Canary - 72.0.3588.0 From your schedule it looks like you are in a meeting right now. Come by my desk for a demo.

I'm OOO today - I'll come by tomorrow if that works for you.

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

commit 3746d2cb185d26d6045c7e282b29e1dc0bbbc00f
Author: David Bokan <bokan@chromium.org>
Date: Mon Oct 29 20:21:35 2018

Add tracing for browser controls state

Bug:  861618 
Change-Id: If5f91df67aa66a1292073f051fb551b9193b7078
Reviewed-on: https://chromium-review.googlesource.com/c/1305297
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603603}
[modify] https://crrev.com/3746d2cb185d26d6045c7e282b29e1dc0bbbc00f/content/renderer/render_view_impl.cc

I've looked a bit closer at this. It looks like there's something going wrong in the browser controls layout code, in the browser process. I've attached traces taken on Canary and a ToT build - the ToT seems to always work but Canary doesn't. In the trace, you can see check for the events: RenderViewImpl::UpdateBrowserControlsState. The values are:

1 - Shown
2 - Hidden
3 - Both (unconstrained/unchanged)

In the bad (canary) trace, we seem to only every receive UpdateBrowserControlsState with the hidden constraint. In the good trace, we receive hidden and shown as expected. When we go back to the main page and the controls should be hidden, the page is actually in a weird state: you can't scroll all the way to the bottom and the overscroll glow starts well below the bottom of the viewport. To me this indicates that the controls are "hidden" but the entire layer hierarchy is shifted down incorrectly. So the bottom of the content is off-screen.

pkotwicz@, I don't know that layout code at all and it seems specific to WebAPKs, could you take a look from there?

I'm adding more diagnostics in https://chromium-review.googlesource.com/c/chromium/src/+/1351272/ since it's strange to me that we can hide the bar on scrolling when we navigate to "Fork on Github" even though we've only ever gotten a "Hidden" constraint. I'll try to find out what's going on there (it's hard since the issue doesn't repro on a local build). However, it looks like the main issue here is in Browser-side code.

Deleted: trace_trace_bad.json.gz 691 KB

trace_trace_bad.json.gz 691 KB Download

Deleted: trace_trace_good.json.gz 874 KB

trace_trace_good.json.gz 874 KB Download

+some folks who may be familiar with that (mdjones, mheikal)

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

commit 8443de775d957b7a816c6b3a22451148b0b874f6
Author: David Bokan <bokan@chromium.org>
Date: Mon Nov 26 22:34:12 2018

Add TRACE logging for browser controls

Needed to debug unexplained behavior seen in browser controls that
don't reproduce in a local Chrome build.

Bug:  861618 
Change-Id: I9a6769cd47a3329313628bd714039ec532fc3d94
Reviewed-on: https://chromium-review.googlesource.com/c/1351272
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610963}
[modify] https://crrev.com/8443de775d957b7a816c6b3a22451148b0b874f6/cc/input/browser_controls_offset_manager.cc
[modify] https://crrev.com/8443de775d957b7a816c6b3a22451148b0b874f6/cc/trees/layer_tree_impl.cc

- Last time we talked (a couple of weeks ago) I was able to repro with an official ToT build

- I think that it would be useful to add diagnostic code inside of Tab#updateBrowserControlsState(). If that function is called with an incorrect value (I think that this is what you are suggesting) then that would be useful to know.

- The layout code is not specific to WebAPKs and is controlled by renderer.

- The UI code calls Tab#updateFullscreenEnabledState() to push the state it wants to the renderer.
  - In the case of pressing "X" the function is called by TabStateBrowserControlsVisibilityDelegate#onDidFinishNavigation()
- The y position of the top controls is set via ChromeFullscreenManager#updateVisuals() which is called by
  TabBrowserControlsOffsetHelper#onOffsetsChanged() which is called by
      ViewAndroid::OnTopControlsChanged()
- The y position of the web contents is updated via Layout#getUpdatedSceneLayer() which is called by LayoutManager#getUpdatedActiveSceneLayer() which is called by CompositorView::UpdateLayerTreeHost()
- The y position of the web contents comes from ChromeFullscreenManager#getTopControlOffset() which is set via ViewAndroid::OnTopControlsChanged()

The y position of the web contents and the y position of the top controls are updated at different times. This might be why the entire layer heirarchy is shifted down.

Correction: The y position of the web contents is updated via StaticSceneLayer#update() which is called by Layout#updateSceneLayer()

> Last time we talked (a couple of weeks ago) I was able to repro with an official ToT build

My build is official ToT, here's my GN args, am I missing anything?

use_goma = true
goma_dir = "/home/bokan/tools/goma"
ffmpeg_branding = "Chrome"
is_chrome_branded = true
is_debug = false
is_official_build = true
proprietary_codecs = true
strip_absolute_paths_from_debug_symbols = true
symbol_level = 1 
target_os = "android"

I still build chrome_public_apk, is there maybe a different target?

Yes, the chrome_apk build target is more official than chrome_public_apk

Your GN args look right.

You can specify android_channel="canary" to change the app icon colour. I don't think that would matter though.

Hah...android_channel="canary" seems to have done it...

For the sake of clarity, can you reproduce the bug locally now?

Yes - I'm trying to track where things go wrong. The renderer appears to be sending the correct (hidden) browser control ratio to the browser, I'm following that through the locations you mentioned in #48 

OK - I think I've found the issue. RenderWidgetHostViewAndroid::UpdateControls keeps track of the last offset sent to the Java layer and only sends an update if the values changed.

Unfortunately, when we navigate back/press "X", the RenderWidgetHostViewAndroid gets swapped out so the prev_top_shown_pix is cleared. If this happens before we get the new compositor frame with top_shown_pix=0, RWHVA is out of sync with the state of the top controls in the Java layer.

This issue appears to have some history.

- In https://crrev.com/c/961265/ mthiesse@ hit this in VR and so added a flag to ensure we pass the update if we haven't seen one before (i.e. always pass the first update after the RWHVA is swapped out).
- This broke interstitials in  bug 825765  so the flag was used only in VR mode.
- From  bug 825765  it looks like interstitial code was fixed but reader mode still relies on the first update being ignored (bug 853686).

1) It looks like the "did anything change" check is at the wrong level, the Java code should keep track of state and early-out if nothing changed
2) We'd need to first fix reader mode (and maybe other locations) before we can fix #1 or remove the VR-only guard

Does someone know who owns reader mode and could help with that? Reader mode appears busted for me on ToT (always get blank unscrollable page) so I can't confirm if its still an issue.

+wychen +mdjones ?

I thought the java code did actually early out if nothing changed ChromeFullscreenManager#setPositionsForTab:
https://chromium.googlesource.com/chromium/src/+/4b8211d386f19b359471e0d43b8d7fd7f425fffe/chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java#667

Hm, maybe that's enough? There's other code in TabBrowserControlsOffsetHelper#onOffsetsChanged that may or may not be trivial though: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/tab/TabBrowserControlsOffsetHelper.java?type=cs&q=onOffsetsChanged+file:java&g=0&l=132

I think that method just caches the current values so they can be restored if you switch back to a tab.

We likely should be tracking whether the renderer associated with the tab dies while the tab is no longer in the foreground to clear those values, but if you aren't seeing jumps when you're switching tabs specifically when the renderer has died, then I think that function is a pretty dumb passthrough.

ToT reader mode is working now, when I try reverting the patch landed as a result of bug 853686 (https://crrev.com/c/1105124) [the reader-mode issue] I can't reproduce the bug. I'll land a revert, hopefully that should 

Looks like pshmakov@ beat me to the punch and reverted the early-outs in 908037. pkotwicz@, can you double-check that everything behaves as expected now?

I checked and this bug is fixed as of 73.0.3637.0 Thanks bokan@ for all of your work! I have requested a merge for  Issue 908037 

Yep - I'm also seeing the fix locally on the case I could reliably repro. This has been a thorn in our side for some time with occasional reports. Thanks, David!!

For people trying to reproduce the bug, the bug only occurred with Monochrome arm64 builds. I have only tested on Pixel phones but am not sure if this is limited to Pixel phones. I have tested that this is limited to arm64 and to Monochrome (The bug did not occur with arm32 Monochrome or with arm64 Chrome.apk)

 Issue 911089  has been merged into this issue.

I'm going to mark this as fixed since there's nothing left to do AFAIK. Please speak up if you're still seeing similar issues in builds newer than 73.0.3637.0.

Unfortunately, there's still a problem with reader mode, if "reader mode in cct" flag is disabled: https://bugs.chromium.org/p/chromium/issues/detail?id=922388