I saw this issue on this test url only -http://video.webmfiles.org/big-buck-bunny_trailer.webm.

Youtube videos are fine when exiting from full screen.

Yes, I think this issue will exist for many urls.
kravula@ - Could you help us find out the exact CL where it regressed?

This repros on both online and offline videos. YouTube works fine, but espncricinfo.com is broken. I will look for some more sites.

I've hit that bug using imdb the other day too. It is either an issue with
screen orientation or fullscreen.

 Issue 698357  has been merged into this issue.

Good build-57.0.2953.0
Bad Build -57.0.2954.0
Regression range: https://chromium.googlesource.com/chromium/src/+log/57.0.2953.0..57.0.2954.0?pretty=fuller&n=10000

Per CL bisect in-progress

Good commit: 439071
Bad commit: 439072
Suspect CL:https://chromium.googlesource.com/chromium/src/+/1a6bd5037fc10e45646715d675e63146d23fc28d

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

commit 9c5e6c49a3367fb995f81353536df1e971f2905b
Author: Alex Mineer <amineer@chromium.org>
Date: Sat Mar 11 01:26:37 2017

Revert "[android] Make RWHVAndroid a BeginFrameObserver."

This reverts commit 1a6bd5037fc10e45646715d675e63146d23fc28d.

Reverting on branch 2954 only to confirm bisect of this CL as the
root cause of linked bug.

BUG= 698315 

Cr-Commit-Position: refs/branch-heads/2954@{#3}
Cr-Branched-From: 806ee4e75cbdd4254d8b240426f357c98c8b2601-refs/heads/master@{#439279}

[modify] https://crrev.com/9c5e6c49a3367fb995f81353536df1e971f2905b/content/browser/android/synchronous_compositor_browser_filter.cc
[modify] https://crrev.com/9c5e6c49a3367fb995f81353536df1e971f2905b/content/browser/android/synchronous_compositor_browser_filter.h
[modify] https://crrev.com/9c5e6c49a3367fb995f81353536df1e971f2905b/content/browser/renderer_host/compositor_impl_android.cc
[modify] https://crrev.com/9c5e6c49a3367fb995f81353536df1e971f2905b/content/browser/renderer_host/compositor_impl_android.h
[modify] https://crrev.com/9c5e6c49a3367fb995f81353536df1e971f2905b/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/9c5e6c49a3367fb995f81353536df1e971f2905b/content/browser/renderer_host/render_widget_host_view_android.h
[modify] https://crrev.com/9c5e6c49a3367fb995f81353536df1e971f2905b/ui/android/DEPS
[modify] https://crrev.com/9c5e6c49a3367fb995f81353536df1e971f2905b/ui/android/delegated_frame_host_android.cc
[modify] https://crrev.com/9c5e6c49a3367fb995f81353536df1e971f2905b/ui/android/delegated_frame_host_android.h
[modify] https://crrev.com/9c5e6c49a3367fb995f81353536df1e971f2905b/ui/android/window_android.cc
[modify] https://crrev.com/9c5e6c49a3367fb995f81353536df1e971f2905b/ui/android/window_android.h
[modify] https://crrev.com/9c5e6c49a3367fb995f81353536df1e971f2905b/ui/android/window_android_compositor.h
[modify] https://crrev.com/9c5e6c49a3367fb995f81353536df1e971f2905b/ui/android/window_android_observer.h

Assigning to eseckler@ and CC'ing reviewers.  Can you folks please take a look at this ASAP?  The steps in c#0 I believe should reproduce things, and you can use builds 57.0.2954.0 to reproduce (and 57.0.2954.3 with the suspect CL reverted to not reproduce, I did a test build on an older version as I'm sure there's no way this is reverting cleanly any longer).

Things don't look impactful in older builds, but if you view video in our M57 stable candidate (57.0.2987.102, or just get the latest beta from the Play Store) you will see that viewing videos on espncricinfo.com, or viewing videos from tumblr (with native controls present) things reproduce much more frequently (I'm assuming since landscape orientation is forced).  It seems like we'll see this in the wild and it's a bad experience (you have to zoom way back out, if you can figure out what happened).

Is there anything we can to do fix this?  We were planning to go to stable on Monday so there is negative time here.

This sounds like a bug we've looked into before: https://crbug.com/679497.

There, it also originally bisected down to my change (the one you tried to revert), but was actually due to timing changes caused by this patch:

https://chromium.googlesource.com/chromium/src/+/e1d42d636990425056ede44086ef49a1f8c6a0a5

It seemed to have been fixed when foolip@ reverted this change. See also the related bug: https://crbug.com/679281.

I'm not sure why it popped up again now.

This seems to be caused by a race between the renderer-side updates on exiting fullscreen and the browser telling the renderer about a resize caused by the fullscreen mode change.

For me, the page is always scaled in to a pageScaleFactor of 4.0x. I believe that this factor is set here [1], which is executed when the browser receives the ViewMsg_Resize from the renderer [2] after exiting fullscreen. (The browser sends this message after it received FrameHostMsg_ToggleFullscreen from the renderer during exitFullscreen().)

The relevant code in [1]:
  float newPageScaleFactor =
      m_oldPageScaleFactor / m_oldMinimumPageScaleFactor *
      m_pageScaleConstraintsSet.finalConstraints().minimumScale;

For some reason, the minimum scale changes from 0.25 to 1.0. As a result of that, the pageScaleFactor is set to 4.0.

What I don't understand yet is why this happens, or why the minimum scale changes from 0.25 to 1.0.

For my local build, I also cannot reproduce reliably locally. Not sure if relevant, but if the problem occurs, I can observe the following interleaving:
  FullscreenController::exitFullscreen()
  didExitFullscreen()
  updatePageScaleConstraints(remove=true)
  fullscreenElementChanged()
  RenderWidgetHostView::WasResized()
  didUpdateLayout()
  RenderViewImpl::OnResize

If it does not occur, I can observe this interleaving instead:
  FullscreenController::exitFullscreen()
  RenderWidgetHostView::WasResized()
  didExitFullscreen()
  updatePageScaleConstraints(remove=true)
  fullscreenElementChanged()
  didUpdateLayout()
  RenderWidgetHostView::WasResized()
  RenderViewImpl::OnResize

[1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/RotationViewportAnchor.cpp?l=164
[2] RenderViewImpl::OnResize which leads to: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebViewImpl.cpp?l=1926

We're thinking this might be related to this logic in WebViewImpl::resizeWithBrowserControls, which was originally added in [1].

  bool isRotation =
      page()->settings().getMainFrameResizesAreOrientationChanges() &&
      m_size.width && contentsSize().width() && newSize.width != m_size.width &&
      !m_fullscreenController->isFullscreen();
  [..]
  if (isRotation) {
    RotationViewportAnchor anchor(*view, visualViewport, viewportAnchorCoords,
                                  pageScaleConstraintsSet());
    resizeViewWhileAnchored(browserControlsHeight, browserControlsShrinkLayout);
  } else {
    ResizeViewportAnchor::ResizeScope resizeScope(*m_resizeViewportAnchor);
    resizeViewWhileAnchored(browserControlsHeight, browserControlsShrinkLayout);
  }

It seems that the calculation of isRotation tries to exclude fullscreen-related changes, but doesn't hit when exiting fullscreen (anymore?) b/c m_fullscreenController->isFullscreen() is already false. We're trying to confirm if m_fullscreenController->isFullscreen() was true on exit when things worked fine.

[1] https://codereview.chromium.org/405263002

When the bug doesn't occur, isFullscreen=false and isRotation=true in the snippet in #17. So the problem doesn't seem to lie there but rather in the fact that the min page scale is 0.25 before WebViewImpl::resizeWithBrowserControls.

Not *before* but *when* WebViewImpl::resizeWithBrowserControls is called. `m_oldPageScaleFactor` and `m_oldMinimumPageScaleFactor` are set using `page()->visualViewport()` and `page()->pageScaleConstraintsSet()` when `RotationViewportAnchor` is created. For some reason, the page has incorrect value at this point.

Some logging of order of calls:

[if things go well:]

[ERROR:FullscreenController.cpp(185)] exitFullscreen
[ERROR:FullscreenController.cpp(194)] exitFullscreen noearlyout
[ERROR:render_widget_host_impl.cc(722)] WasResized size: 732x412 mode: 1
[ERROR:VisualViewport.cpp(107)] setSize size: "732x412"
[ERROR:VisualViewport.cpp(107)] setSize size: "732x412"
[ERROR:VisualViewport.cpp(255)] didSetScaleOrLocation scale: 1
[ERROR:FullscreenController.cpp(98)] didExitFullscreen
[ERROR:FullscreenController.cpp(107)] didExitFullscreen noearlyout
[ERROR:FullscreenController.cpp(283)] updatePageScaleConstraints noearlyout remove: 1
[ERROR:FullscreenController.cpp(205)] fullscreenElementChanged noearlyout
[ERROR:WebViewImpl.cpp(2995)] setPageScaleFactor scale: 1 clamped: 1
[ERROR:FullscreenController.cpp(267)] didUpdateLayout
[ERROR:FullscreenController.cpp(272)] didUpdateLayout noearlyout current: (1,"0x0","0,0"), new: (1,"0x0","0,0")
[ERROR:WebViewImpl.cpp(2995)] setPageScaleFactor scale: 1 clamped: 1
[ERROR:VisualViewport.cpp(255)] didSetScaleOrLocation scale: 1
[ERROR:render_widget_host_impl.cc(722)] WasResized size: 732x412 mode: 1
[ERROR:VisualViewport.cpp(107)] setSize size: "732x412"
[ERROR:VisualViewport.cpp(255)] didSetScaleOrLocation scale: 1
[ERROR:render_widget_host_impl.cc(722)] WasResized size: 412x660 mode: 1
[ERROR:WebViewImpl.cpp(1916)] resizeWithBrowserControls isRotation: 1 isFullscreen: 0
[ERROR:VisualViewport.cpp(107)] setSize size: "412x660"
[ERROR:VisualViewport.cpp(110)] setSize noearlyout
[ERROR:FullscreenController.cpp(242)] updateSize
[ERROR:WebViewImpl.cpp(2995)] setPageScaleFactor scale: 1 clamped: 1
[ERROR:VisualViewport.cpp(147)] mainFrameDidChangeSize noearlyout
[ERROR:VisualViewport.cpp(255)] didSetScaleOrLocation scale: 1
[ERROR:FullscreenController.cpp(267)] didUpdateLayout
[ERROR:RotationViewportAnchor.cpp(167)] restoreToAnchor old_min: 1 new min: 1 old scale: 1 new scale: 1
[ERROR:VisualViewport.cpp(255)] didSetScaleOrLocation scale: 1

[if things go bad:]

[ERROR:FullscreenController.cpp(185)] exitFullscreen
[ERROR:FullscreenController.cpp(194)] exitFullscreen noearlyout
[ERROR:render_widget_host_impl.cc(722)] WasResized size: 732x412 mode: 1
[ERROR:VisualViewport.cpp(107)] setSize size: "732x412"
[ERROR:VisualViewport.cpp(107)] setSize size: "732x412"
[ERROR:VisualViewport.cpp(255)] didSetScaleOrLocation scale: 1
[ERROR:FullscreenController.cpp(98)] didExitFullscreen
[ERROR:FullscreenController.cpp(107)] didExitFullscreen noearlyout
[ERROR:FullscreenController.cpp(283)] updatePageScaleConstraints noearlyout remove: 1
[ERROR:FullscreenController.cpp(205)] fullscreenElementChanged noearlyout
[ERROR:render_widget_host_impl.cc(722)] WasResized size: 732x412 mode: 1
[ERROR:VisualViewport.cpp(107)] setSize size: "732x412"
[ERROR:VisualViewport.cpp(255)] didSetScaleOrLocation scale: 1
[ERROR:render_widget_host_impl.cc(722)] WasResized size: 412x660 mode: 1
[ERROR:WebViewImpl.cpp(1916)] resizeWithBrowserControls isRotation: 1 isFullscreen: 0
[ERROR:VisualViewport.cpp(107)] setSize size: "412x660"
[ERROR:VisualViewport.cpp(110)] setSize noearlyout
[ERROR:FullscreenController.cpp(242)] updateSize
[ERROR:WebViewImpl.cpp(2995)] setPageScaleFactor scale: 1 clamped: 1
[ERROR:VisualViewport.cpp(147)] mainFrameDidChangeSize noearlyout
[ERROR:VisualViewport.cpp(255)] didSetScaleOrLocation scale: 1
[ERROR:FullscreenController.cpp(267)] didUpdateLayout
[ERROR:FullscreenController.cpp(272)] didUpdateLayout noearlyout current: (1,"0x0","0,0"), new: (1,"0x0","0,0")
[ERROR:WebViewImpl.cpp(2995)] setPageScaleFactor scale: 1 clamped: 1
[ERROR:VisualViewport.cpp(255)] didSetScaleOrLocation scale: 1
[ERROR:RotationViewportAnchor.cpp(167)] restoreToAnchor old_min: 0.25 new min: 1 old scale: 1 new scale: 4
[ERROR:VisualViewport.cpp(255)] didSetScaleOrLocation scale: 4

Thus, when things go well, a layout occurs before resizeWithBrowserControls. During the layout, FullscreenController::didUpdateLayout is called, which removes the fullscreen page scale constraints and recalculates them.

When things go badly, this layout happens during the resize (during WebViewImpl::resizeViewWhileAnchored). Thus, FullscreenController::didUpdateLayout wasn't called yet when the anchor is set.

Could it have to do with that?

Yes, I think it is related. The call we are missing exactly is refreshPageScaleFactorAfterLayout(). On my test page for example, the minimumScale is set to 0.25 (overridden to 1.0 when fullscreen). When the method is called, it's set back to 1.0 because PageScaleConstraintsSet::adjustFinalConstraintsToContentsSize is called.

Not sure what the right fix should be. We could call ::refreshPageScaleFactorAfterLayout() or PageScaleConstraintsSet::adjustFinalConstraintsToContentsSize() I think.

regarding my comment in #21: actually, I was confusing FullScreenController::didUpdateLayout and FullscreenController::updatePageScaleConstraints.

So yes, it has to do with page scale constraints and the order of layouting, but not with didUpdateLayout.

Sounds like this normally works b/c the layout calls refreshPageScaleFactorAfterLayout() and happens before WebViewImpl::resizeWithBrowserControls.

Question is if we can do what Mounir suggests and simply call refreshPageScaleFactorAfterLayout() without doing a layout, or if we need the layout to actually happen. In which case, could we force a layout before the resize, e.g. during/after FullscreenController::didExitFullscreen or FullscreenController::updatePageScaleConstraints?

The "...AfterLayout" kind of suggests the function is expecting layout to be non-dirty when it's called :) Forcing a layout when exiting fullscreen mode doesn't sound too bad to me, but not sure how often updatePageScaleConstraints gets called.

I'd add it at the end of didExitFullscreen, since that's the only place where updatePageScaleConstraints is called with remove=false.

Fix suggested in #27 would be this patch: https://codereview.chromium.org/2750653002/

Doing additional layout proves to cause some needsLayout failures on the trybots.  I came up with a very safe mitigation up for review at https://codereview.chromium.org/2745313002

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

commit 80da37a9ae40452f920fdb8a5bbd55454f7e560c
Author: aelias <aelias@chromium.org>
Date: Mon Mar 13 21:34:42 2017

Avoid rotation anchor during transitional fullscreen states.

FullscreenController::isFullscreen is called exclusively by
WebViewImpl::resizeWithBrowserControls to exclude use of rotation
anchor.  The reason is that anchors incorrectly interpret the initial
offset restoration in FullscreenController::didUpdateLayout as a
layout-induced change.  Rotation anchors in particular tend to
snap page scale factor to extreme values as a result.

During the observed case, fullscreen state is still NeedsScrollAndStateRestore.
It's reasonable to consider every state in the machine other than "Initial"
(non-fullscreen) to count as fullscreen.

This change still leaves incorrect restoration behavior due to the resize
anchor, but the symptoms are much severe in that case because it doesn't
touch page scale.

NOTRY=true
BUG= 698315 

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

[modify] https://crrev.com/80da37a9ae40452f920fdb8a5bbd55454f7e560c/third_party/WebKit/Source/web/FullscreenController.h
[modify] https://crrev.com/80da37a9ae40452f920fdb8a5bbd55454f7e560c/third_party/WebKit/Source/web/WebViewImpl.cpp

Your change meets the bar and is auto-approved for M58. Please go ahead and merge the CL to branch 3029 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), bhthompson@(cros), govind@(desktop)

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

This bug requires manual review: Only 0 days from stable, we might already have a stable candidate build
Please contact the milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

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

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

commit b302bdacad3416b46336402624990a9a4de1a0d2
Author: Alexandre Elias <aelias@chromium.org>
Date: Mon Mar 13 21:40:56 2017

Avoid rotation anchor during transitional fullscreen states.

FullscreenController::isFullscreen is called exclusively by
WebViewImpl::resizeWithBrowserControls to exclude use of rotation
anchor.  The reason is that anchors incorrectly interpret the initial
offset restoration in FullscreenController::didUpdateLayout as a
layout-induced change.  Rotation anchors in particular tend to
snap page scale factor to extreme values as a result.

During the observed case, fullscreen state is still NeedsScrollAndStateRestore.
It's reasonable to consider every state in the machine other than "Initial"
(non-fullscreen) to count as fullscreen.

This change still leaves incorrect restoration behavior due to the resize
anchor, but the symptoms are much severe in that case because it doesn't
touch page scale.

NOTRY=true
BUG= 698315 

Review-Url: https://codereview.chromium.org/2745313002
Cr-Commit-Position: refs/heads/master@{#456495}
(cherry picked from commit 80da37a9ae40452f920fdb8a5bbd55454f7e560c)

Review-Url: https://codereview.chromium.org/2746083003 .
Cr-Commit-Position: refs/branch-heads/3029@{#167}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/b302bdacad3416b46336402624990a9a4de1a0d2/third_party/WebKit/Source/web/FullscreenController.h
[modify] https://crrev.com/b302bdacad3416b46336402624990a9a4de1a0d2/third_party/WebKit/Source/web/WebViewImpl.cpp

Your change meets the bar and is auto-approved for M58. Please go ahead and merge the CL to branch 3029 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), bhthompson@(cros), govind@(desktop)

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

Approved for M57 branch 2987.

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

commit 93a3cca341d450d38da434c75b00f1c170fed363
Author: Alexandre Elias <aelias@chromium.org>
Date: Mon Mar 13 22:13:26 2017

Avoid rotation anchor during transitional fullscreen states.

FullscreenController::isFullscreen is called exclusively by
WebViewImpl::resizeWithBrowserControls to exclude use of rotation
anchor.  The reason is that anchors incorrectly interpret the initial
offset restoration in FullscreenController::didUpdateLayout as a
layout-induced change.  Rotation anchors in particular tend to
snap page scale factor to extreme values as a result.

During the observed case, fullscreen state is still NeedsScrollAndStateRestore.
It's reasonable to consider every state in the machine other than "Initial"
(non-fullscreen) to count as fullscreen.

This change still leaves incorrect restoration behavior due to the resize
anchor, but the symptoms are much severe in that case because it doesn't
touch page scale.

NOTRY=true
BUG= 698315 

Review-Url: https://codereview.chromium.org/2745313002
Cr-Commit-Position: refs/heads/master@{#456495}
(cherry picked from commit 80da37a9ae40452f920fdb8a5bbd55454f7e560c)

Review-Url: https://codereview.chromium.org/2747063002 .
Cr-Commit-Position: refs/branch-heads/2987@{#817}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/93a3cca341d450d38da434c75b00f1c170fed363/third_party/WebKit/Source/web/FullscreenController.h
[modify] https://crrev.com/93a3cca341d450d38da434c75b00f1c170fed363/third_party/WebKit/Source/web/WebViewImpl.cpp

Works as per expected behavior, Issue fixed on Latest M57 and M58 Builds.