Fixed race condition during window creation

Currently browser was not waiting for renderer part of the navigation
before calling a callback on chrome.app.create. It assumed that
the mojo ipc will be handled quickly, which is not true in cases of
overloaded renderer(e.g kiosk app with many backgroud apps).

Bug: 943710
Change-Id: I92effd2cd5daca8d95e1b7ad04bf47b6ef6dfb88
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1655396
Commit-Queue: Anatoliy Potapchuk <apotapchuk@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#668576}

extensions/browser/api/app_window/app_window_api.cc

@@ -423,18 +423,18 @@ ExtensionFunction::ResponseAction AppWindowCreateFunction::Run() {
   // Delay sending the response until the newly created window has been told to
   // navigate, and blink has been correctly initialized in the renderer.
   // SetOnFirstCommitOrWindowClosedCallback() will respond asynchronously.
-  app_window->SetOnFirstCommitOrWindowClosedCallback(base::Bind(
-      &AppWindowCreateFunction::OnAppWindowReadyToCommitFirstNavigationOrClosed,
-      this, base::Passed(&result_arg)));
+  app_window->SetOnDidFinishFirstNavigationCallback(base::BindOnce(
+      &AppWindowCreateFunction::OnAppWindowFinishedFirstNavigationOrClosed,
+      this, std::move(result_arg)));
   return RespondLater();
 }
 
-void AppWindowCreateFunction::OnAppWindowReadyToCommitFirstNavigationOrClosed(
+void AppWindowCreateFunction::OnAppWindowFinishedFirstNavigationOrClosed(
     ResponseValue result_arg,
-    bool ready_to_commit) {
+    bool did_finish) {
   DCHECK(!did_respond());
 
-  if (!ready_to_commit) {
+  if (!did_finish) {
     Respond(Error(app_window_constants::kPrematureWindowClose));
     return;
   }

extensions/browser/api/app_window/app_window_api.h

@@ -26,8 +26,8 @@ class AppWindowCreateFunction : public UIThreadExtensionFunction {
   ResponseAction Run() override;
 
  private:
-  void OnAppWindowReadyToCommitFirstNavigationOrClosed(ResponseValue result_arg,
-                                                       bool ready_to_commit);
+  void OnAppWindowFinishedFirstNavigationOrClosed(ResponseValue result_arg,
+                                                  bool did_finish);
 
   bool GetBoundsSpec(
       const extensions::api::app_window::CreateWindowOptions& options,

extensions/browser/app_window/app_window.cc

@@ -470,26 +470,18 @@ void AppWindow::RenderViewCreated(content::RenderViewHost* render_view_host) {
   app_delegate_->RenderViewCreated(render_view_host);
 }
 
-void AppWindow::SetOnFirstCommitOrWindowClosedCallback(
-    FirstCommitOrWindowClosedCallback callback) {
-  DCHECK(on_first_commit_or_window_closed_callback_.is_null());
-  on_first_commit_or_window_closed_callback_ = std::move(callback);
+void AppWindow::SetOnDidFinishFirstNavigationCallback(
+    DidFinishFirstNavigationCallback callback) {
+  DCHECK(on_did_finish_first_navigation_callback_.is_null());
+  on_did_finish_first_navigation_callback_ = std::move(callback);
 }
 
-void AppWindow::OnReadyToCommitFirstNavigation() {
-  // Execute renderer-side setup now that there is a renderer process assigned
-  // to the navigation. We must wait until this point in time in the navigation.
-  if (on_first_commit_or_window_closed_callback_.is_null())
+void AppWindow::OnDidFinishFirstNavigation() {
+  // We need to call it exactly once.
+  if (on_did_finish_first_navigation_callback_.is_null())
     return;
-  // It is important that the callback executes after the calls to
-  // WebContentsObserver::ReadyToCommitNavigation have been processed. The
-  // CommitNavigation IPC that will properly set up the renderer will only be
-  // sent after these, and it must be sent before the callback gets to run,
-  // hence the use of PostTask.
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE,
-      base::BindOnce(std::move(on_first_commit_or_window_closed_callback_),
-                     true /* ready_to_commit */));
+  std::move(on_did_finish_first_navigation_callback_)
+      .Run(true /* did_finish */);
 }
 
 void AppWindow::OnNativeClose() {
@@ -498,11 +490,11 @@ void AppWindow::OnNativeClose() {
   // Dispatch "OnClosed" event by default.
   bool send_onclosed = true;
 
-  // Run pending |on_first_commit_or_window_closed_callback_| so that
+  // Run pending |on_did_finish_first_navigation_callback_| so that
   // AppWindowCreateFunction can respond with an error properly.
-  if (!on_first_commit_or_window_closed_callback_.is_null()) {
-    std::move(on_first_commit_or_window_closed_callback_)
-        .Run(false /* ready_to_commit */);
+  if (!on_did_finish_first_navigation_callback_.is_null()) {
+    std::move(on_did_finish_first_navigation_callback_)
+        .Run(false /* did_finish */);
 
     send_onclosed = false;  // No "OnClosed" event on window creation error.
   }

extensions/browser/app_window/app_window.h

@@ -255,17 +255,16 @@ class AppWindow : public content::WebContentsDelegate,
   // is on startup and from within UpdateWindowTitle().
   base::string16 GetTitle() const;
 
-  // |callback| will be called when the first navigation in the window is
-  // ready to commit or when the window goes away before that. |ready_to_commit|
-  // argument of the |callback| is set to true for the former case and false for
-  // the later.
-  using FirstCommitOrWindowClosedCallback =
-      base::OnceCallback<void(bool ready_to_commit)>;
-  void SetOnFirstCommitOrWindowClosedCallback(
-      FirstCommitOrWindowClosedCallback callback);
-
-  // Called when the first navigation in the window is ready to commit.
-  void OnReadyToCommitFirstNavigation();
+  // |callback| will be called when the first navigation was completed or window
+  // is closed before that. |did_finish| argument of the |callback| is set to
+  // true for the former case and false for the latter.
+  using DidFinishFirstNavigationCallback =
+      base::OnceCallback<void(bool did_finish)>;
+  void SetOnDidFinishFirstNavigationCallback(
+      DidFinishFirstNavigationCallback callback);
+
+  // Called when first navigation was completed.
+  void OnDidFinishFirstNavigation();
 
   // Call to notify ShellRegistry and delete the window. Subclasses should
   // invoke this method instead of using "delete this".
@@ -587,9 +586,9 @@ class AppWindow : public content::WebContentsDelegate,
   // race condition of loading custom app icon and app content simultaneously.
   bool window_ready_ = false;
 
-  // PlzNavigate: this is called when the first navigation is ready to commit or
-  // when the window is closed.
-  FirstCommitOrWindowClosedCallback on_first_commit_or_window_closed_callback_;
+  // PlzNavigate: this is called when the navigation is finished on both browser
+  // and renderer sides.
+  DidFinishFirstNavigationCallback on_did_finish_first_navigation_callback_;
 
   base::WeakPtrFactory<AppWindow> image_loader_ptr_factory_;
 

extensions/browser/app_window/app_window_contents.cc

@@ -96,9 +96,10 @@ bool AppWindowContentsImpl::OnMessageReceived(
   return handled;
 }
 
-void AppWindowContentsImpl::ReadyToCommitNavigation(
+void AppWindowContentsImpl::DidFinishNavigation(
     content::NavigationHandle* handle) {
-  host_->OnReadyToCommitFirstNavigation();
+  // The callback inside app_window will be moved after the first call.
+  host_->OnDidFinishFirstNavigation();
 }
 
 void AppWindowContentsImpl::UpdateDraggableRegions(

extensions/browser/app_window/app_window_contents.h

@@ -46,7 +46,7 @@ class AppWindowContentsImpl : public AppWindowContents,
   // content::WebContentsObserver
   bool OnMessageReceived(const IPC::Message& message,
                          content::RenderFrameHost* sender) override;
-  void ReadyToCommitNavigation(content::NavigationHandle* handle) override;
+  void DidFinishNavigation(content::NavigationHandle* handle) override;
 
   void UpdateDraggableRegions(content::RenderFrameHost* sender,
                               const std::vector<DraggableRegion>& regions);