Ensure fullscreen mode is exited for same-site navigations.

Ensure fullscreen mode is exited for same-site navigations.

Resolves a fullscreen-within-tab bug where navigation using the Back button doesn't exit fullscreen.  This would result in the browser (WebContentsDelegate) erroneously showing a page with the "fullscreen in your tab" UI layout.

Furthermore, this change will cause the previous renderer to be notified that it has been force-exited from fullscreen.  Should the user revisit using the Back/Forward/History actions, the renderer will therefore be in the correct non-fullscreen state.

Added a new unit test to web_contents_impl_unittest.cc to make sure this problem doesn't regress.

BUG=356951, 347232
TEST=Repro steps in bug 356951 result in fullscreen exit.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261087

[miu, 2014-03-29 02:55:44]

creis: PTAL.

While r254923 fixed the problem for cross-site navigations (and anything else
that causes a RFH to be swapped out), it didn't fix the issue for same-site
navigations where the RFH is reused.

[Charlie Reis, 2014-03-31 20:21:36]

Sorry for the delay; I was sheriff last week.

I'm not clear what the conditions for exiting fullscreen are, and I see
potential problems with both this and http://crrev.com/254923.

When should fullscreen mode exit?
1) When any navigation (same-origin or not) starts?
2) When any navigation (same-origin or not) commits?
3) When a cross-origin navigation commits?
4) When a cross-process navigation commits?

I'm guessing it's 2 or 3.  Navigations that start may not commit (e.g.,
downloads), so 1 probably isn't right.  Also, the SwappedOut logic from your
earlier CL seems like it's in the wrong place-- I would be surprised if there's
anything process-model specific about when to exit fullscreen, so 4 shouldn't be
right either.

[miu, 2014-03-31 21:40:40]

On 2014/03/31 20:21:36, Charlie Reis wrote:
> I'm not clear what the conditions for exiting fullscreen are, and I see
> potential problems with both this and http://crrev.com/254923.

This is "new frontier" here.  The problem is that fullscreen-within-tab has
introduced previously meaningless semantics: Main-frame navigation was never
possible during Flash/HTML5 fullscreen.  So, I'm trying to fill-in what makes
sense now, given my limited understanding of how navigation works in Chrome.  I
would like to hear your opinions; here are mine:

> When should fullscreen mode exit?
> 1) When any navigation (same-origin or not) starts?

That's too early.  I believe we should give the page onbeforeunload handlers a
chance to say "no" before triggering a forced exit from fullscreen.  I think
it's ideal to force-exit fullscreen at the last possible moment while the
renderer (and browser window) are still configured to the old navigation entry.

> 2) When any navigation (same-origin or not) commits?

That's too late.  If NavigationControllerImpl keeps history entries around
(therefore keeping render frames alive), the render frames need to be exited
from fullscreen so that they do not think they are in fullscreen when becoming
current again (e.g., user clicks back button).  Furthermore, the delegate (i.e.,
browser window UI) needs to be informed so it can reconfigure the UI before the
navigation destination is committed and shown, as no page loads assuming it is
starting out in fullscreen.

> 3) When a cross-origin navigation commits?
> 4) When a cross-process navigation commits?

I think whether the navigation is same-site, cross-origin, or cross-process
doesn't matter.  In all cases, we want fullscreen to exit before committing the
navigation.

> Also, the SwappedOut logic from your
> earlier CL seems like it's in the wrong place-- I would be surprised if
there's
> anything process-model specific about when to exit fullscreen

IIUC, I agree.  Exiting fullscreen on navigations should make this unnecessary. 
Unless: Do you know of any cases where a RVH/RFH would be swapped out outside of
the navigation procedure?  r254923 also accounts for the special case of
just-crashed renderers.

All said, I think this change is doing the right thing: NavigatorImpl is calling
WebContentsImpl::DidStartNavigationToPendingEntry() at the point-of-no-return,
but just before any actions have been taken to tear-down the old "connections"
and start showing the destination renderer.  In addition, this code point must
be executed regardless of cross-origin/process or same-site navigating.

WDYT?

Also, keep in mind that this is something I will need to merge into the M35
branch.  So, if this is the right solution but maybe the wrong factoring, I can
address that in a later change.

[Charlie Reis, 2014-03-31 22:50:00]

On 2014/03/31 21:40:40, miu wrote:
> On 2014/03/31 20:21:36, Charlie Reis wrote:
> > I'm not clear what the conditions for exiting fullscreen are, and I see
> > potential problems with both this and http://crrev.com/254923.
> 
> This is "new frontier" here.  The problem is that fullscreen-within-tab has
> introduced previously meaningless semantics: Main-frame navigation was never
> possible during Flash/HTML5 fullscreen.  So, I'm trying to fill-in what makes
> sense now, given my limited understanding of how navigation works in Chrome. 
I
> would like to hear your opinions; here are mine:
> 
> > When should fullscreen mode exit?
> > 1) When any navigation (same-origin or not) starts?
> 
> That's too early.  I believe we should give the page onbeforeunload handlers a
> chance to say "no" before triggering a forced exit from fullscreen.  I think
> it's ideal to force-exit fullscreen at the last possible moment while the
> renderer (and browser window) are still configured to the old navigation
entry.
> 
> > 2) When any navigation (same-origin or not) commits?
> 
> That's too late.  If NavigationControllerImpl keeps history entries around
> (therefore keeping render frames alive), the render frames need to be exited
> from fullscreen so that they do not think they are in fullscreen when becoming
> current again (e.g., user clicks back button).  Furthermore, the delegate
(i.e.,
> browser window UI) needs to be informed so it can reconfigure the UI before
the
> navigation destination is committed and shown, as no page loads assuming it is
> starting out in fullscreen.
> 
> > 3) When a cross-origin navigation commits?
> > 4) When a cross-process navigation commits?
> 
> I think whether the navigation is same-site, cross-origin, or cross-process
> doesn't matter.  In all cases, we want fullscreen to exit before committing
the
> navigation.

I agree so far.  It sounds like we're talking about the point immediately before
the new page commits.

> 
> > Also, the SwappedOut logic from your
> > earlier CL seems like it's in the wrong place-- I would be surprised if
> there's
> > anything process-model specific about when to exit fullscreen
> 
> IIUC, I agree.  Exiting fullscreen on navigations should make this
unnecessary. 
> Unless: Do you know of any cases where a RVH/RFH would be swapped out outside
of
> the navigation procedure?  r254923 also accounts for the special case of
> just-crashed renderers.

We'll only see a process swap when a navigation occurs, so you should be fine
there.  And yes, watching for the process to crash seems reasonable, so that we
don't stay in fullscreen with a sad tab.

> All said, I think this change is doing the right thing: NavigatorImpl is
calling
> WebContentsImpl::DidStartNavigationToPendingEntry() at the point-of-no-return,
> but just before any actions have been taken to tear-down the old "connections"
> and start showing the destination renderer.  In addition, this code point must
> be executed regardless of cross-origin/process or same-site navigating.
> 
> WDYT?

DidStartNavigationToPendingEntry is not the point-of-no-return.  That's called
from Navigator::NavigateToEntry, which means we're just starting to make the
network request-- it could still be slow, canceled by the user, turn out to be
download, or turn out to be a 204 that doesn't commit.  In addition, we don't
hit that call for every navigation.  Renderer-initiated navigations (e.g., link
clicks) don't take that path.  In the worst case, we don't have any network
requests and we just hear the commit (e.g., data URLs).

I think we would want to take the action just before the page commits in
NavigatorImpl::DidNavigate.  At that point, the commit message has arrived from
the renderer process but we haven't committed it to the session history or (in
the case of cross-process navigations) swapped processes yet.

There's currently no call out to WebContents for this, but there's already a
call to TakeScreenshot at this point in time, for similar reasons.  I'm guessing
that's where this should occur.  Code:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...


> Also, keep in mind that this is something I will need to merge into the M35
> branch.  So, if this is the right solution but maybe the wrong factoring, I
can
> address that in a later change.

Thanks for the heads up, though let's focus on getting the right fix first.

[miu, 2014-04-01 03:45:32]

On 2014/03/31 22:50:00, Charlie Reis wrote:
> There's currently no call out to WebContents for this, but there's already a
> call to TakeScreenshot at this point in time, for similar reasons.  I'm
guessing
> that's where this should occur.  Code:

Ah, yes!  That's a much better code point to tap into.  I followed the existing
pattern of DidNavigateMainFramePostCommit(), and added a
NavigatorDelegate::Did...PreCommit().  Then, in WebContentsImpl, the pre-commit
task of force-exiting fullscreen is handled.

I also removed the force-exit code from WebContentsImpl::SwappedOut(), as
discussed.

PTAL.

[Charlie Reis, 2014-04-01 23:12:38]

Great, much better.  LGTM!

Maybe change the CL subject to "Ensure fullscreen mode is exited for all
navigations," now that we're unifying the cross-process and same-site cases?

https://codereview.chromium.org/218093002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/218093002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl.cc:407: controller_->TakeScreenshot();
I agree that we shouldn't do it here, but I may move this stuff to
DidNavigateMainFramePreCommit in a future CL as well.

https://codereview.chromium.org/218093002/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/218093002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:2248:
old_render_view_host->ExitFullscreen();
GetRenderViewHost() will never return null in this context (since we're hearing
an IPC reply and thus have initialized it), so you can just call it here rather
than storing it in old_render_view_host.

https://codereview.chromium.org/218093002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:2250: // TODO(creis): How to
handle subframes that go fullscreen?
You may know the answer to this, actually.  Can subframes go fullscreen, or is
this a moot point?  If not, we can remove the TODO.

[miu, 2014-04-01 23:39:07]

The CQ bit was checked by miu@chromium.org

[miu, 2014-04-01 23:39:35]

Thanks for the review!  :)

https://codereview.chromium.org/218093002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/218093002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl.cc:407: controller_->TakeScreenshot();
On 2014/04/01 23:12:38, Charlie Reis wrote:
> I agree that we shouldn't do it here, but I may move this stuff to
> DidNavigateMainFramePreCommit in a future CL as well.

Yeah, I almost did that.  ;-)  Feel free to use me as a code reviewer, or if I
notice it's still this way a few weeks from now, I might make the change.

https://codereview.chromium.org/218093002/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/218093002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:2248:
old_render_view_host->ExitFullscreen();
On 2014/04/01 23:12:38, Charlie Reis wrote:
> GetRenderViewHost() will never return null in this context (since we're
hearing
> an IPC reply and thus have initialized it), so you can just call it here
rather
> than storing it in old_render_view_host.

Done.

https://codereview.chromium.org/218093002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:2250: // TODO(creis): How to
handle subframes that go fullscreen?
On 2014/04/01 23:12:38, Charlie Reis wrote:
> You may know the answer to this, actually.  Can subframes go fullscreen, or is
> this a moot point?  If not, we can remove the TODO.

Removed TODO.  I just looked at the W3C spec, and it looks like any subdocument
entering fullscreen mode should bubble up the request to ancestor documents.  I
interpret this as "a subframe entering fullscreen will cause the main frame to
enter fullscreen."

[commit-bot: I haz the power, 2014-04-01 23:39:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miu@chromium.org/218093002/60001

[commit-bot: I haz the power, 2014-04-02 00:06:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-02 00:06:13]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[miu, 2014-04-02 00:40:06]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2014-04-02 00:47:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miu@chromium.org/218093002/60001

[commit-bot: I haz the power, 2014-04-02 02:12:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miu@chromium.org/218093002/60001

[commit-bot: I haz the power, 2014-04-02 06:43:00]

Change committed as 261087

[mario.prada, 2014-04-02 12:59:35]

On 2014/04/02 06:43:00, I haz the power (commit-bot) wrote:
> Change committed as 261087

On 2014/04/02 06:20:40, I haz the power (commit-bot) wrote:
> Change committed as 170638

According to the flakiness dashboard, this patch looks suspicious to have broken
a bunch of fullscreen related tests for debug builds of blink:

  http://goo.gl/6EEy3u

I've looked at the crash log for Blink commit r170638 (clearly the only one in
the intersection of commit ranges for blink) and couldn't see any obvious
relationship between that crash and that Blink commit. However, by looking at
the intersection of commit ranges for chromium [1], I've found this commit which
seems suspicious because of being related to fullscreen, which is the cause of
the crash in Blink (in WebCore::FullscreenElementStack::webkitExitFullscreen()
[FullscreenElementStack.cpp : 284 + 0x33])

So, I'll be filing a bug shortly to add the test to TestExpectations shortly,
since it's causing noise in the try bots at the moment. Please consider taking a
look to this when you have time, thanks!

[1]
http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog.html?url=/tr...