PlzNavigate: fix timing issue in app window creation

PlzNavigate: fix timing issue in app window creation

This CL fixes a timing bug in which a newly created app window would try to
register a ScriptContext before being told to commit a navigation. This would
result in an incorrect frame url being passed to the ScriptContextSet, and a
wrong ContextType to be computed from it. With this CL, when browser-side
navigation is enabled, sending the response to the app.window.create is
deferred until the navigation in the newly created frame is ready to commit.

BUG=504347
Committed: https://crrev.com/efca29ea8d6161c50c7e289f8d5ed7b33723bc41
Cr-Commit-Position: refs/heads/master@{#349290}

[clamy, 2015-09-15 18:05:10]

clamy@chromium.org changed reviewers:
+ creis@chromium.org, nasko@chromium.org, rdevlin.cronin@chromium.org

[clamy, 2015-09-15 18:08:01]

@naskoe, creis, rdcronin: PTAL

This CL fixes the issue of app windows with PlzNavigate that I mentionned by
mail. The fix here is to delay sending the response to the creator of the app
window until the navigation is ready to commit (which properly initializes the
renderer). It is still guaranteed that the response will be sent before the
OnLoad event, because the request from the renderer to get the body of the
response will be blocked in the network stack until the callback has executed
and it gets unblocked by the AppWindowContents.

[nasko, 2015-09-16 20:19:36]

Looks good overall, though I think the naming of the method can be a bit better.

https://codereview.chromium.org/1340163002/diff/40001/content/public/browser/...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1340163002/diff/40001/content/public/browser/...
content/public/browser/web_contents_observer.h:147: virtual void
AboutToCommitNavigation(NavigationHandle* navigation_handle) {}
I think ReadyToCommitNavigation is a bit more precise. I find the "about to do
something" a bit more ambiguous.

https://codereview.chromium.org/1340163002/diff/40001/extensions/browser/api/...
File extensions/browser/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/1340163002/diff/40001/extensions/browser/api/...
extensions/browser/api/app_window/app_window_api.cc:375:
::switches::kEnableBrowserSideNavigation)) {
nit: Is the extra :: in front of switches required?

https://codereview.chromium.org/1340163002/diff/40001/extensions/browser/app_...
File extensions/browser/app_window/app_window.h (right):

https://codereview.chromium.org/1340163002/diff/40001/extensions/browser/app_...
extensions/browser/app_window/app_window.h:250: void OnFirstCommit();
OnFirstCommit implies that the commit has happened. OnReadyToCommit or something
similar?

https://codereview.chromium.org/1340163002/diff/40001/extensions/browser/app_...
File extensions/browser/app_window/app_window_contents.cc (right):

https://codereview.chromium.org/1340163002/diff/40001/extensions/browser/app_...
extensions/browser/app_window/app_window_contents.cc:57: LOG(ERROR) <<
"Navigating to " << url_.spec();
Is this log intentional?

[clamy, 2015-09-16 21:32:35]

Thanks!

https://codereview.chromium.org/1340163002/diff/40001/content/public/browser/...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1340163002/diff/40001/content/public/browser/...
content/public/browser/web_contents_observer.h:147: virtual void
AboutToCommitNavigation(NavigationHandle* navigation_handle) {}
On 2015/09/16 20:19:36, nasko (slow to review) wrote:
> I think ReadyToCommitNavigation is a bit more precise. I find the "about to do
> something" a bit more ambiguous.

Agreed. Changed the name.

https://codereview.chromium.org/1340163002/diff/40001/extensions/browser/api/...
File extensions/browser/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/1340163002/diff/40001/extensions/browser/api/...
extensions/browser/api/app_window/app_window_api.cc:375:
::switches::kEnableBrowserSideNavigation)) {
On 2015/09/16 20:19:36, nasko (slow to review) wrote:
> nit: Is the extra :: in front of switches required?

Yes, otherwise it tries to resolve the namespace to extensions::switches.

https://codereview.chromium.org/1340163002/diff/40001/extensions/browser/app_...
File extensions/browser/app_window/app_window.h (right):

https://codereview.chromium.org/1340163002/diff/40001/extensions/browser/app_...
extensions/browser/app_window/app_window.h:250: void OnFirstCommit();
On 2015/09/16 20:19:36, nasko (slow to review) wrote:
> OnFirstCommit implies that the commit has happened. OnReadyToCommit or
something
> similar?

Done.

https://codereview.chromium.org/1340163002/diff/40001/extensions/browser/app_...
File extensions/browser/app_window/app_window_contents.cc (right):

https://codereview.chromium.org/1340163002/diff/40001/extensions/browser/app_...
extensions/browser/app_window/app_window_contents.cc:57: LOG(ERROR) <<
"Navigating to " << url_.spec();
On 2015/09/16 20:19:36, nasko (slow to review) wrote:
> Is this log intentional?

No it's not :). Removed it.

[nasko, 2015-09-16 21:37:50]

content/ LGTM with one last note.

https://codereview.chromium.org/1340163002/diff/60001/content/public/browser/...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1340163002/diff/60001/content/public/browser/...
content/public/browser/web_contents_observer.h:146: // navigation.
One thing I forgot to mention. Let's add a comment here saying that observers
that want to initialize any renderer side structures/state before the
RenderFrame is navigated, should use this method as opposed to
DidCommitNavigation, which is after the fact.

[Devlin, 2015-09-16 21:44:04]

https://codereview.chromium.org/1340163002/diff/60001/extensions/browser/app_...
File extensions/browser/app_window/app_window.cc (right):

https://codereview.chromium.org/1340163002/diff/60001/extensions/browser/app_...
extensions/browser/app_window/app_window.cc:448: on_first_commit_callback_ =
callback;
Can we DCHECK(on_first_commit_callback_.is_null()) here?

https://codereview.chromium.org/1340163002/diff/60001/extensions/browser/app_...
extensions/browser/app_window/app_window.cc:458: //
WebContentsObserver::AboutToCommitNavigation have been processed. The
Two notes:
- AboutToCommitNavigation seems to have been renamed.
- This worries me.  AboutToCommitNavigation() is a new signal, and we're already
abusing it by saying "this is near what we want, but not quite, but we hope that
it'll be ready after some other tasks run."  Is there any way to make this a
little bit better?

https://codereview.chromium.org/1340163002/diff/60001/extensions/browser/app_...
extensions/browser/app_window/app_window.cc:463: on_first_commit_callback_);
nit: base::ResetAndReturn(&on_first_commit_callback_)

https://codereview.chromium.org/1340163002/diff/60001/extensions/browser/app_...
File extensions/browser/app_window/app_window.h (right):

https://codereview.chromium.org/1340163002/diff/60001/extensions/browser/app_...
extensions/browser/app_window/app_window.h:247: void
SetOnFirstCommitCallback(base::Callback<void(void)> callback);
const base::Closure& is cleaner. :)

https://codereview.chromium.org/1340163002/diff/60001/extensions/browser/app_...
extensions/browser/app_window/app_window.h:566: base::Callback<void(void)>
on_first_commit_callback_;
base::Closure

https://codereview.chromium.org/1340163002/diff/60001/extensions/browser/app_...
File extensions/browser/app_window/app_window_contents.cc (right):

https://codereview.chromium.org/1340163002/diff/60001/extensions/browser/app_...
extensions/browser/app_window/app_window_contents.cc:125:
host_->OnReadyToCommit();
OnReadyToCommit() implies it might be called on *every* commit.  Maybe include a
"first" somewhere?

[clamy, 2015-09-16 22:19:39]

Thanks!

https://codereview.chromium.org/1340163002/diff/60001/content/public/browser/...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1340163002/diff/60001/content/public/browser/...
content/public/browser/web_contents_observer.h:146: // navigation.
On 2015/09/16 21:37:50, nasko (slow to review) wrote:
> One thing I forgot to mention. Let's add a comment here saying that observers
> that want to initialize any renderer side structures/state before the
> RenderFrame is navigated, should use this method as opposed to
> DidCommitNavigation, which is after the fact.

Done.

https://codereview.chromium.org/1340163002/diff/60001/extensions/browser/app_...
File extensions/browser/app_window/app_window.cc (right):

https://codereview.chromium.org/1340163002/diff/60001/extensions/browser/app_...
extensions/browser/app_window/app_window.cc:448: on_first_commit_callback_ =
callback;
On 2015/09/16 21:44:04, Devlin wrote:
> Can we DCHECK(on_first_commit_callback_.is_null()) here?

Done.

https://codereview.chromium.org/1340163002/diff/60001/extensions/browser/app_...
extensions/browser/app_window/app_window.cc:458: //
WebContentsObserver::AboutToCommitNavigation have been processed. The
On 2015/09/16 21:44:04, Devlin wrote:
> Two notes:
> - AboutToCommitNavigation seems to have been renamed.
> - This worries me.  AboutToCommitNavigation() is a new signal, and we're
already
> abusing it by saying "this is near what we want, but not quite, but we hope
that
> it'll be ready after some other tasks run."  Is there any way to make this a
> little bit better?

Updated the comment.

As for the ReadyToCommitNavigation , we had planned on adding this call to the
WebContentsObserver interface for some time, as it will be needed by other
components such as DevTools (which require to have it happen exactly at the time
it happens there). At the same time, we'd like to avoid having to add a signal
after we've sent the IPC telling the renderer to commit the navigation, because
we want to limit the number of signals available to the WebContentsObserver.

The PostTask here is to ensure that any IPC sent to the renderer will be sent
after the CommitNavigation IPC. Given the way IPC queuing work, this also
ensures that the extension IPC will be processed after the CommitNavigation one,
therefore the Renderer will have been properly initialized.

https://codereview.chromium.org/1340163002/diff/60001/extensions/browser/app_...
extensions/browser/app_window/app_window.cc:463: on_first_commit_callback_);
On 2015/09/16 21:44:04, Devlin wrote:
> nit: base::ResetAndReturn(&on_first_commit_callback_)

Done.

https://codereview.chromium.org/1340163002/diff/60001/extensions/browser/app_...
File extensions/browser/app_window/app_window.h (right):

https://codereview.chromium.org/1340163002/diff/60001/extensions/browser/app_...
extensions/browser/app_window/app_window.h:247: void
SetOnFirstCommitCallback(base::Callback<void(void)> callback);
On 2015/09/16 21:44:04, Devlin wrote:
> const base::Closure& is cleaner. :)

Done.

https://codereview.chromium.org/1340163002/diff/60001/extensions/browser/app_...
extensions/browser/app_window/app_window.h:566: base::Callback<void(void)>
on_first_commit_callback_;
On 2015/09/16 21:44:04, Devlin wrote:
> base::Closure

Done.

https://codereview.chromium.org/1340163002/diff/60001/extensions/browser/app_...
File extensions/browser/app_window/app_window_contents.cc (right):

https://codereview.chromium.org/1340163002/diff/60001/extensions/browser/app_...
extensions/browser/app_window/app_window_contents.cc:125:
host_->OnReadyToCommit();
On 2015/09/16 21:44:04, Devlin wrote:
> OnReadyToCommit() implies it might be called on *every* commit.  Maybe include
a
> "first" somewhere?

Done.

[Devlin, 2015-09-16 22:37:14]

lgtm

https://codereview.chromium.org/1340163002/diff/60001/extensions/browser/app_...
File extensions/browser/app_window/app_window.cc (right):

https://codereview.chromium.org/1340163002/diff/60001/extensions/browser/app_...
extensions/browser/app_window/app_window.cc:458: //
WebContentsObserver::AboutToCommitNavigation have been processed. The
On 2015/09/16 22:19:39, clamy wrote:
> On 2015/09/16 21:44:04, Devlin wrote:
> > Two notes:
> > - AboutToCommitNavigation seems to have been renamed.
> > - This worries me.  AboutToCommitNavigation() is a new signal, and we're
> already
> > abusing it by saying "this is near what we want, but not quite, but we hope
> that
> > it'll be ready after some other tasks run."  Is there any way to make this a
> > little bit better?
> 
> Updated the comment.
> 
> As for the ReadyToCommitNavigation , we had planned on adding this call to the
> WebContentsObserver interface for some time, as it will be needed by other
> components such as DevTools (which require to have it happen exactly at the
time
> it happens there). At the same time, we'd like to avoid having to add a signal
> after we've sent the IPC telling the renderer to commit the navigation,
because
> we want to limit the number of signals available to the WebContentsObserver.
> 
> The PostTask here is to ensure that any IPC sent to the renderer will be sent
> after the CommitNavigation IPC. Given the way IPC queuing work, this also
> ensures that the extension IPC will be processed after the CommitNavigation
one,
> therefore the Renderer will have been properly initialized.

But even after all these IPCs, we still have a guarantee that the callback from
the extension API call (basically this task, +some IPCs) will definitely finish
before the onload handler for the window, right?

https://codereview.chromium.org/1340163002/diff/80001/content/public/browser/...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1340163002/diff/80001/content/public/browser/...
content/public/browser/web_contents_observer.h:125: //
DidRedirect/Commit/FinishNavigation/AboutToCommitNavigation related to
s/AboutTo/ReadyTo

https://codereview.chromium.org/1340163002/diff/80001/extensions/browser/app_...
File extensions/browser/app_window/app_window_contents.cc (right):

https://codereview.chromium.org/1340163002/diff/80001/extensions/browser/app_...
extensions/browser/app_window/app_window_contents.cc:125:
host_->OnReadyToCommitFirstNavigation();
nit: instead of early returning, just put the call in the if statement (and
invert it)

[clamy, 2015-09-16 23:08:25]

Thanks!

https://codereview.chromium.org/1340163002/diff/60001/extensions/browser/app_...
File extensions/browser/app_window/app_window.cc (right):

https://codereview.chromium.org/1340163002/diff/60001/extensions/browser/app_...
extensions/browser/app_window/app_window.cc:458: //
WebContentsObserver::AboutToCommitNavigation have been processed. The
On 2015/09/16 22:37:14, Devlin wrote:
> On 2015/09/16 22:19:39, clamy wrote:
> > On 2015/09/16 21:44:04, Devlin wrote:
> > > Two notes:
> > > - AboutToCommitNavigation seems to have been renamed.
> > > - This worries me.  AboutToCommitNavigation() is a new signal, and we're
> > already
> > > abusing it by saying "this is near what we want, but not quite, but we
hope
> > that
> > > it'll be ready after some other tasks run."  Is there any way to make this
a
> > > little bit better?
> > 
> > Updated the comment.
> > 
> > As for the ReadyToCommitNavigation , we had planned on adding this call to
the
> > WebContentsObserver interface for some time, as it will be needed by other
> > components such as DevTools (which require to have it happen exactly at the
> time
> > it happens there). At the same time, we'd like to avoid having to add a
signal
> > after we've sent the IPC telling the renderer to commit the navigation,
> because
> > we want to limit the number of signals available to the WebContentsObserver.
> > 
> > The PostTask here is to ensure that any IPC sent to the renderer will be
sent
> > after the CommitNavigation IPC. Given the way IPC queuing work, this also
> > ensures that the extension IPC will be processed after the CommitNavigation
> one,
> > therefore the Renderer will have been properly initialized.
> 
> But even after all these IPCs, we still have a guarantee that the callback
from
> the extension API call (basically this task, +some IPCs) will definitely
finish
> before the onload handler for the window, right?

Yes I have verified that. Since we block requests from the renderer in the
network stack until the callback gets to execute, the renderer cannot actually
get the body of the response, and doesn't commit until the requests are
unblocked in the network stack. If it cannot commit,then we're sure the onload
handler will not be called.

https://codereview.chromium.org/1340163002/diff/80001/content/public/browser/...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1340163002/diff/80001/content/public/browser/...
content/public/browser/web_contents_observer.h:125: //
DidRedirect/Commit/FinishNavigation/AboutToCommitNavigation related to
On 2015/09/16 22:37:14, Devlin wrote:
> s/AboutTo/ReadyTo

Done.

https://codereview.chromium.org/1340163002/diff/80001/extensions/browser/app_...
File extensions/browser/app_window/app_window_contents.cc (right):

https://codereview.chromium.org/1340163002/diff/80001/extensions/browser/app_...
extensions/browser/app_window/app_window_contents.cc:125:
host_->OnReadyToCommitFirstNavigation();
On 2015/09/16 22:37:14, Devlin wrote:
> nit: instead of early returning, just put the call in the if statement (and
> invert it)

Done.

[clamy, 2015-09-16 23:08:30]

The CQ bit was checked by clamy@chromium.org

[clamy, 2015-09-16 23:08:31]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org,
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1340163002/#ps100001
(title: "Addressed nits")

[commit-bot: I haz the power, 2015-09-16 23:09:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1340163002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1340163002/100001

[commit-bot: I haz the power, 2015-09-17 00:22:17]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-09-17 00:23:13]

Patchset 6 (id:??) landed as
https://crrev.com/efca29ea8d6161c50c7e289f8d5ed7b33723bc41
Cr-Commit-Position: refs/heads/master@{#349290}