PlzNavigate: Fix ordering of onBeforeNavigate and onCreatedNavigationTarget.

chrome/browser/extensions/api/web_navigation/web_navigation_api.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Implements the Chrome Extensions WebNavigation API.
 
 #include "chrome/browser/extensions/api/web_navigation/web_navigation_api.h"
 
 #include "base/lazy_instance.h"
 #include "chrome/browser/chrome_notification_types.h"
@@ skipping 253 matching lines @@
   navigation_state_.FrameHostCreated(new_host);
 }
 
 void WebNavigationTabObserver::DidStartNavigation(
     content::NavigationHandle* navigation_handle) {
   if (navigation_handle->IsSamePage() ||
       !FrameNavigationState::IsValidUrl(navigation_handle->GetURL())) {
     return;
   }
 
-  helpers::DispatchOnBeforeNavigate(navigation_handle);
+  pending_on_before_navigate_event_ =
+      helpers::CreateOnBeforeNavigateEvent(navigation_handle);
+
+  // Only dispatch the onBeforeNavigate event if the associated WebContents
+  // is already added to the tab strip. Otherwise the event should be delayed
+  // and sent after the addition, to preserve the ordering of events.
+  if (ExtensionTabUtil::GetTabById(

[Devlin, 2016/12/02 20:17:11]

I don't love that this is how we check this.  It makes me worried if we ever
- Include unattached tabs in GetTabById()
- Are correctly able to dispatch the oncreated event before the tab is added to
the tab strip

that this will behave incorrectly.  But since we use this logic elsewhere, it
would have to be a bulk update.  Maybe add a TODO to some effect?  WDYT?

[nasko, 2016/12/02 21:40:45]

I think there will be more fallout if we did that than just this instance :).
If we get to that point, I'll be happy to fix this. The reason I'm using it is
so it is consistent with how this code behaves in other places in the file. It
will be easier to grep/fix later if it is consistently using the same pattern
than not.
 
Indeed. Bulk update will be nice. I'm fine adding a TODO, but not sure what the
action should be. Just adding it without explicit and clear action doesn't seem
very useful to me. Open to ideas if you have any.

[Devlin, 2016/12/02 21:58:31]

// TODO(nasko|devlin): We have to do this check because chrome::Navigate()
begins the navigation before the sending the TabAdded notification, and we use
this as an indication of that. It would be best if we instead knew when the tab
was created and immediately sent the created event instead of waiting for the
later TabAdded notification, but this appears to work for now.

[nasko, 2016/12/02 23:41:39]

Done.

+          ExtensionTabUtil::GetTabId(web_contents()),
+          Profile::FromBrowserContext(web_contents()->GetBrowserContext()),

[Devlin, 2016/12/02 20:17:11]

nit: this can take a browser context, so no need for Profile casting.

[nasko, 2016/12/02 21:40:45]

Done.

+          false, NULL, NULL, NULL, NULL)) {

[Devlin, 2016/12/02 20:17:11]

nit: nullptr in new code.

[nasko, 2016/12/02 21:40:45]

Done.

+    DispatchOnBeforeNavigate();
+  }
 }
 
 void WebNavigationTabObserver::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
+  // If there has been a DidStartNavigation call before the tab was ready to
+  // dispatch events, ensure that it is sent before processing the
+  // DidFinishNavigation.
+  // Note: This is exercised by WebNavigationApiTest.TargetBlankIncognito.
+  DispatchOnBeforeNavigate();

[Devlin, 2016/12/02 20:17:11]

Does this not have the same possible race?  Or why wouldn't we have dispatched
this once we dispatched the target created event?

[nasko, 2016/12/02 21:40:45]

This guarantees that if we are going to send a commit, we must have sent the
onBeforeNavigate. The TargetBlankIncognito test ends up properly dispatching the
onCreatedNavigationTarget, but the onBeforeNavigate wasn't being send, since it
arrived after the TabAdded notification. So this is a safeguard to ensure that
if there is anything cached that we missed sending, we will ensure it is sent
before the commit.

[Devlin, 2016/12/02 21:58:31]

If the TabAdded notification is sent before DidStartNavigation, then we *should*
send the event immediately on line 284 (of this patch set).  If the if above
(line 280) is returning false for the incognito test, it's probably because we
pass "false" for "include_incognito" to GetTabById.  We should be able to flip
that to true, since we know there will only be a single tab with that id and
don't care if it's incognito or not.

[nasko, 2016/12/02 23:41:39]

The BrowserContext passed in should already be the incognito one, so the tab
should be found just fine.

It turns out that this happens for the very first tab in the incognito window,
not for the latter. Unfortunately, if I move the dispatch to be part of the
TabAdded handling, we are back to broken state. I know this is not ideal, but it
might be the best we can do without fixing chrome::Navigate (which I already
spent at least 2-3 weeks trying to disentangle).

+
   if (navigation_handle->HasCommitted() && !navigation_handle->IsErrorPage()) {
     HandleCommit(navigation_handle);
     return;
   }
 
   HandleError(navigation_handle);
 }
 
 void WebNavigationTabObserver::DocumentLoadedInFrame(
     content::RenderFrameHost* render_frame_host) {
@@ skipping 95 matching lines @@
                                              source_render_frame_host,
                                              new_contents,
                                              url);
 }
 
 void WebNavigationTabObserver::WebContentsDestroyed() {
   g_tab_observer.Get().erase(web_contents());
   registrar_.RemoveAll();
 }
 
+void WebNavigationTabObserver::DispatchOnBeforeNavigate() {
+  if (!pending_on_before_navigate_event_.get())
+    return;
+
+  Profile* profile =
+      Profile::FromBrowserContext(web_contents()->GetBrowserContext());
+  EventRouter* event_router = EventRouter::Get(profile);

[Devlin, 2016/12/02 20:17:11]

nit: use browser context directly.

[nasko, 2016/12/02 21:40:45]

Done.

+  if (profile && event_router)

[Devlin, 2016/12/02 20:17:11]

can profile/context be null?  (Or event router, for that matter?)  If it can, I
think we'll crash on EventRouter::Get().

[nasko, 2016/12/02 21:40:45]

I don't know whether the context can be null, possibly in unit tests? Looking at
other usage, it looks like lots of places use the returned pointer directly, so
doesn't look like it should ever be null.

+    event_router->BroadcastEvent(std::move(pending_on_before_navigate_event_));
+}
+
 void WebNavigationTabObserver::HandleCommit(
     content::NavigationHandle* navigation_handle) {
   bool is_reference_fragment_navigation = IsReferenceFragmentNavigation(
       navigation_handle->GetRenderFrameHost(), navigation_handle->GetURL());
 
   navigation_state_.StartTrackingDocumentLoad(
       navigation_handle->GetRenderFrameHost(), navigation_handle->GetURL(),
       navigation_handle->IsSamePage(),
       false,  // is_error_page
       navigation_handle->IsSrcdoc());
@@ skipping 156 matching lines @@
   return g_factory.Pointer();
 }
 
 void WebNavigationAPI::OnListenerAdded(const EventListenerInfo& details) {
   web_navigation_event_router_.reset(new WebNavigationEventRouter(
       Profile::FromBrowserContext(browser_context_)));
   EventRouter::Get(browser_context_)->UnregisterObserver(this);
 }
 
 }  // namespace extensions