A few fixes for IE CEEE BHO stability:

1. Ensure that the BHO has registered its tab ID before it ever sends any tab-related
events to the broker.
2. Ensure that the tabs.onUpdated and tabs.onRemoved events are only fired if the
tabs.onCreated event was fired first.
3. Ensure that the ceee.onTabUnmapped event is only fired if the tab ID was registered.

BUG=none
TEST=ie_unittests.exe

Review URL: http://codereview.chromium.org/5378002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@67426 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 185 insertions, 86 deletions

diff --git a/ceee/ie/plugin/bho/browser_helper_object.cc b/ceee/ie/plugin/bho/browser_helper_object.cc
index 78aea5e..e926adc 100644
--- a/ceee/ie/plugin/bho/browser_helper_object.cc
+++ b/ceee/ie/plugin/bho/browser_helper_object.cc
@@ -646,34 +646,68 @@ HRESULT BrowserHelperObject::CreateChromeFrameHost() {
                                                chrome_frame_host_.Receive());
 }
 
-HRESULT BrowserHelperObject::FireOnCreatedEvent(BSTR url) {
+void BrowserHelperObject::FireOnCreatedEvent(BSTR url) {
   DCHECK(url != NULL);
   DCHECK(tab_window_ != NULL);
   DCHECK(!fired_on_created_event_);
+  // Avoid sending double-creation events. Also only fire the event if we're
+  // NOT in full-tab mode, because by design, extensions are not supposed to be
+  // aware of full-tab Chrome Frame tabs.
+  if (fired_on_created_event_ || full_tab_chrome_frame_)
+    return;
+
   HRESULT hr = tab_events_funnel().OnCreated(tab_window_, url,
       lower_bound_ready_state_ == READYSTATE_COMPLETE);
   fired_on_created_event_ = SUCCEEDED(hr);
-  DCHECK(SUCCEEDED(hr)) << "Failed to fire the tab.onCreated event " <<
+  DCHECK(SUCCEEDED(hr)) << "Failed to fire the tabs.onCreated event " <<
       com::LogHr(hr);
-  return hr;
 }
 
-HRESULT BrowserHelperObject::FireOnRemovedEvent() {
+void BrowserHelperObject::FireOnUpdatedEvent(BSTR url,
+                                             READYSTATE ready_state) {
   DCHECK(tab_window_ != NULL);
-  HRESULT hr =
-      tab_events_funnel().OnRemoved(tab_window_);
-  DCHECK(SUCCEEDED(hr)) << "Failed to fire the tab.onRemoved event " <<
+  DCHECK(fired_on_created_event_);
+  // Only fire the event if the creation event was already fired for the
+  // tab.
+  if (!fired_on_created_event_)
+    return;
+
+  // The onCreated event should not have been fired if we are in full-tab mode.
+  DCHECK(!full_tab_chrome_frame_);
+  HRESULT hr = tab_events_funnel().OnUpdated(tab_window_, url, ready_state);
+  DCHECK(SUCCEEDED(hr)) << "Failed to fire the tabs.onUpdated event " <<
       com::LogHr(hr);
-  return hr;
 }
 
-HRESULT BrowserHelperObject::FireOnUnmappedEvent() {
+void BrowserHelperObject::FireOnRemovedEvent() {
   DCHECK(tab_window_ != NULL);
-  DCHECK(tab_id_ != kInvalidChromeSessionId);
+  // Only fire the event if the creation event was already fired for the
+  // tab.
+  if (!fired_on_created_event_)
+    return;
+
+  // The onCreated event should not have been fired if we are in full-tab mode.
+  DCHECK(!full_tab_chrome_frame_);
+  HRESULT hr = tab_events_funnel().OnRemoved(tab_window_);
+  DCHECK(SUCCEEDED(hr)) << "Failed to fire the tabs.onRemoved event " <<
+      com::LogHr(hr);
+  // We've now negated the onCreated event, as far as extensions are
+  // concerned.
+  fired_on_created_event_ = false;
+}
+
+void BrowserHelperObject::FireOnUnmappedEvent() {
+  DCHECK(tab_window_ != NULL);
+  // Only send the event to the broker if the tab ID was actually mapped to
+  // begin with. This may not happen if the BHO is being torn down before the
+  // Chrome Frame instance was ready to provide a session ID.
+  if (tab_id_ == kInvalidChromeSessionId)
+    return;
+
   HRESULT hr = tab_events_funnel().OnTabUnmapped(tab_window_, tab_id_);
   DCHECK(SUCCEEDED(hr)) << "Failed to fire the ceee.onTabUnmapped event " <<
       com::LogHr(hr);
-  return hr;
+  tab_id_ = kInvalidChromeSessionId;
 }
 
 void BrowserHelperObject::CloseAll(IContentScriptNativeApi* instance) {
@@ -997,7 +1031,7 @@ bool BrowserHelperObject::BrowserContainsChromeFrame(IWebBrowser2* browser) {
 HRESULT BrowserHelperObject::AttachBrowserHandler(IWebBrowser2* webbrowser,
     IFrameEventHandler** handler) {
   // We're not attached yet, figure out whether we're attaching
-  // to the top-level browser or a frame, and looukup the parentage
+  // to the top-level browser or a frame, and lookup the parentage
   // in the latter case.
   ScopedWebBrowser2Ptr parent_browser;
   HRESULT hr = S_OK;
@@ -1033,18 +1067,21 @@ HRESULT BrowserHelperObject::AttachBrowserHandler(IWebBrowser2* webbrowser,
       bool is_chrome_frame = BrowserContainsChromeFrame(webbrowser);
 
       if (is_chrome_frame) {
-        fired_on_created_event_ = true;
+        // We have navigated to a full tab Chrome Frame page. If the previous
+        // page was an IE tab, then the FireOnRemovedEvent call below will send
+        // out a tabs.onRemoved event so that extensions will believe the tab
+        // is dead.
+        FireOnRemovedEvent();
+        // The code must preserve an invariant: If full_tab_chrome_frame_ is
+        // true, then fired_on_created_event_ must always be false, because
+        // we never want to fire events when in full-tab mode.
+        DCHECK(!fired_on_created_event_);
         full_tab_chrome_frame_ = true;
-        // Send a tabs.onRemoved event to make the extension believe the tab is
-        // dead.
-        hr = FireOnRemovedEvent();
-        DCHECK(SUCCEEDED(hr));
       } else if (document_is_mshtml) {
         // We know it's MSHTML. We check if the last page was chrome frame.
         if (full_tab_chrome_frame_) {
-          // This will send a tabs.onCreated event later to balance the
-          // onRemoved event above.
-          fired_on_created_event_ = false;
+          // Check that the invariant described above has been preserved.
+          DCHECK(!fired_on_created_event_);
           full_tab_chrome_frame_ = false;
         }
       }
@@ -1130,29 +1167,18 @@ void BrowserHelperObject::HandleNavigateComplete(IWebBrowser2* webbrowser,
       // At this point, we should have all the tab windows created,
       // including the proper lower bound ready state set just before,
       // so setup the tab info if it has not been set yet.
-      if (!fired_on_created_event_) {
-        hr = FireOnCreatedEvent(url);
-        DCHECK(SUCCEEDED(hr)) << "Failed to fire tab created event " <<
-            com::LogHr(hr);
-      }
+      if (!fired_on_created_event_)
+        FireOnCreatedEvent(url);
 
       // The onUpdate event usually gets fired after the onCreated,
       // which is fired from FireOnCreatedEvent above.
-      DCHECK(tab_window_ != NULL);
-      hr = tab_events_funnel().OnUpdated(tab_window_, url,
-                                         lower_bound_ready_state_);
-      DCHECK(SUCCEEDED(hr)) << "Failed to fire tab updated event " <<
-          com::LogHr(hr);
+      FireOnUpdatedEvent(url, lower_bound_ready_state_);
 
       // If this is a hash change, we manually fire the OnUpdated for the
       // complete ready state as we don't receive ready state notifications
       // for hash changes.
-      if (is_hash_change) {
-        hr = tab_events_funnel().OnUpdated(tab_window_, url,
-                                           READYSTATE_COMPLETE);
-        DCHECK(SUCCEEDED(hr)) << "Failed to fire tab updated event " <<
-            com::LogHr(hr);
-      }
+      if (is_hash_change)
+        FireOnUpdatedEvent(url, READYSTATE_COMPLETE);
     }
   }
 }
@@ -1315,8 +1341,27 @@ HRESULT BrowserHelperObject::OnReadyStateChanged(READYSTATE ready_state) {
     }
   }
 
-  return HandleReadyStateChanged(lower_bound_ready_state_,
-                                 new_lowest_ready_state);
+  if (EnsureTabId() == false) {
+    deferred_tab_id_call_.push_back(NewRunnableMethod(
+        this, &BrowserHelperObject::OnReadyStateChangedImpl,
+        lower_bound_ready_state_, new_lowest_ready_state));
+  } else {
+    OnReadyStateChangedImpl(lower_bound_ready_state_, new_lowest_ready_state);
+  }
+  return S_OK;
+}
+
+void BrowserHelperObject::OnReadyStateChangedImpl(READYSTATE old_state,
+                                                  READYSTATE new_state) {
+  if (old_state == new_state)
+    return;
+
+  // Remember the new lowest ready state as our current one.
+  lower_bound_ready_state_ = new_state;
+
+  // Fire the event if the new ready state got us to or away from complete.
+  if (old_state == READYSTATE_COMPLETE || new_state == READYSTATE_COMPLETE)
+    FireOnUpdatedEvent(NULL, new_state);
 }
 
 HRESULT BrowserHelperObject::GetReadyState(READYSTATE* ready_state) {
@@ -1391,21 +1436,6 @@ void BrowserHelperObject::InsertCodeImpl(
   }
 }
 
-HRESULT BrowserHelperObject::HandleReadyStateChanged(READYSTATE old_state,
-                                                     READYSTATE new_state) {
-  if (old_state == new_state)
-    return S_OK;
-
-  // Remember the new lowest ready state as our current one.
-  lower_bound_ready_state_ = new_state;
-
-  if (old_state == READYSTATE_COMPLETE || new_state == READYSTATE_COMPLETE) {
-    // The new ready state got us to or away from complete, so fire the event.
-    DCHECK(tab_window_ != NULL);
-    return tab_events_funnel().OnUpdated(tab_window_, NULL, new_state);
-  }
-  return S_OK;
-}
 
 HRESULT BrowserHelperObject::GetMatchingUserScriptsCssContent(
     const GURL& url, bool require_all_frames, std::string* css_content) {
diff --git a/ceee/ie/plugin/bho/browser_helper_object.h b/ceee/ie/plugin/bho/browser_helper_object.h
index 265ce86..8433ca7 100644
--- a/ceee/ie/plugin/bho/browser_helper_object.h
+++ b/ceee/ie/plugin/bho/browser_helper_object.h
@@ -209,6 +209,7 @@ class ATL_NO_VTABLE BrowserHelperObject
   void OnNavigateErrorImpl(const ScopedWebBrowser2Ptr& webbrowser,
                            const CComBSTR& url,
                            LONG status_code);
+  void OnReadyStateChangedImpl(READYSTATE old_state, READYSTATE new_state);
   void InsertCodeImpl(const CComBSTR& code,
                       const CComBSTR& file,
                       bool all_frames,
@@ -221,8 +222,6 @@ class ATL_NO_VTABLE BrowserHelperObject
                             IFrameEventHandler** handler);
 
   virtual void HandleNavigateComplete(IWebBrowser2* webbrowser, BSTR url);
-  virtual HRESULT HandleReadyStateChanged(READYSTATE old_state,
-                                          READYSTATE new_state);
 
   // Unit testing seam to create the frame event handler.
   virtual HRESULT CreateFrameEventHandler(IWebBrowser2* browser,
@@ -273,13 +272,16 @@ class ATL_NO_VTABLE BrowserHelperObject
   virtual TabEventsFunnel& tab_events_funnel() { return tab_events_funnel_; }
 
   // Fires the tab.onCreated event via the tab event funnel.
-  virtual HRESULT FireOnCreatedEvent(BSTR url);
+  virtual void FireOnCreatedEvent(BSTR url);
+
+  // Fires the tab.onUpdated event via the tab event funnel.
+  virtual void FireOnUpdatedEvent(BSTR url, READYSTATE ready_state);
 
   // Fires the tab.onRemoved event via the tab event funnel.
-  virtual HRESULT FireOnRemovedEvent();
+  virtual void FireOnRemovedEvent();
 
   // Fires the private message to unmap a tab to its BHO.
-  virtual HRESULT FireOnUnmappedEvent();
+  virtual void FireOnUnmappedEvent();
 
   // Loads our manifest and initialize our librarian.
   virtual void LoadManifestFile(const std::wstring& base_dir);
diff --git a/ceee/ie/plugin/bho/browser_helper_object_unittest.cc b/ceee/ie/plugin/bho/browser_helper_object_unittest.cc
index a75d824..771ba15 100644
--- a/ceee/ie/plugin/bho/browser_helper_object_unittest.cc
+++ b/ceee/ie/plugin/bho/browser_helper_object_unittest.cc
@@ -319,12 +319,15 @@ class BrowserHelperObjectTest: public testing::Test {
     }
   }
 
-  void ExpectFireOnRemovedEvent() {
-    EXPECT_CALL(bho_->mock_tab_events_funnel_, OnRemoved(_));
+  // Take the number of times the call is expected as input.
+  void ExpectFireOnRemovedEvent(int times) {
+    EXPECT_CALL(bho_->mock_tab_events_funnel_, OnRemoved(_)).Times(times);
   }
 
-  void ExpectFireOnUnmappedEvent() {
-    EXPECT_CALL(bho_->mock_tab_events_funnel_, OnTabUnmapped(_, _));
+  // Take the number of times the call is expected as input.
+  void ExpectFireOnUnmappedEvent(int times) {
+    EXPECT_CALL(bho_->mock_tab_events_funnel_, OnTabUnmapped(_, _)).
+        Times(times);
   }
 
   static const wchar_t* kUrl1;
@@ -424,8 +427,10 @@ TEST_F(BrowserHelperObjectTest, SetSiteWithBrowserSucceeds) {
       IID_IUnknown, reinterpret_cast<void**>(&set_site)));
   ASSERT_TRUE(set_site == site_keeper_);
 
-  ExpectFireOnRemovedEvent();
-  ExpectFireOnUnmappedEvent();
+  // No teardown events are fired because neither the create event nor the tab
+  // ID mapping occurred.
+  ExpectFireOnRemovedEvent(0);
+  ExpectFireOnUnmappedEvent(0);
   ASSERT_HRESULT_SUCCEEDED(bho_with_site_->SetSite(NULL));
 
   // And check that the connection was severed.
@@ -455,8 +460,43 @@ TEST_F(BrowserHelperObjectTest, OnNavigateCompleteHandled) {
   ExpectTopBrowserNavigation(true, true);
   browser_->FireOnNavigateComplete(browser_, &CComVariant(kUrl1));
 
-  ExpectFireOnRemovedEvent();
-  ExpectFireOnUnmappedEvent();
+  ExpectFireOnRemovedEvent(1);
+  ExpectFireOnUnmappedEvent(1);
+  ASSERT_HRESULT_SUCCEEDED(bho_with_site_->SetSite(NULL));
+}
+
+TEST_F(BrowserHelperObjectTest, OnNavigateCompleteChromeFrameTab) {
+  CreateSite();
+  CreateBrowser();
+  CreateHandler();
+  ExpectChromeFrameGetSessionId();
+
+  // The site needs to return the top-level browser.
+  site_->browser_ = browser_;
+
+  ASSERT_HRESULT_SUCCEEDED(bho_with_site_->SetSite(site_keeper_));
+
+  // First navigate once without Chrome Frame tab.
+  EXPECT_CALL(*bho_, CreateFrameEventHandler(browser_, NULL, NotNull())).
+      WillOnce(DoAll(CopyInterfaceToArgument<2>(handler_keeper_),
+                     Return(S_OK)));
+  EXPECT_CALL(*bho_, BrowserContainsChromeFrame(browser_)).
+      WillOnce(Return(false));
+  ExpectHandleNavigation(handler_, true);
+  ExpectTopBrowserNavigation(true, true);
+  browser_->FireOnNavigateComplete(browser_, &CComVariant(kUrl1));
+
+  // Now navigate with Chrome Frame.
+  EXPECT_CALL(*bho_, CreateFrameEventHandler(browser_, NULL, NotNull())).
+      WillOnce(DoAll(CopyInterfaceToArgument<2>(handler_keeper_),
+                     Return(S_OK)));
+  EXPECT_CALL(*bho_, BrowserContainsChromeFrame(browser_)).
+      WillOnce(Return(true));
+  ExpectFireOnRemovedEvent(1);
+  ExpectHandleNavigation(handler_, true);
+  browser_->FireOnNavigateComplete(browser_, &CComVariant(kUrl1));
+
+  ExpectFireOnUnmappedEvent(1);
   ASSERT_HRESULT_SUCCEEDED(bho_with_site_->SetSite(NULL));
 }
 
@@ -486,8 +526,8 @@ TEST_F(BrowserHelperObjectTest, RenavigationNotifiesUrl) {
               OnUpdated(_, StrEq(kUrl1), READYSTATE_UNINITIALIZED)).Times(1);
   browser_->FireOnNavigateComplete(browser_, &CComVariant(kUrl1));
 
-  ExpectFireOnRemovedEvent();
-  ExpectFireOnUnmappedEvent();
+  ExpectFireOnRemovedEvent(1);
+  ExpectFireOnUnmappedEvent(1);
   ASSERT_HRESULT_SUCCEEDED(bho_with_site_->SetSite(NULL));
 }
 
@@ -514,8 +554,10 @@ TEST_F(BrowserHelperObjectTest, OnNavigateCompleteUnhandled) {
   // Non-BSTR url parameter.
   browser_->FireOnNavigateComplete(browser_, &CComVariant(non_browser));
 
-  ExpectFireOnRemovedEvent();
-  ExpectFireOnUnmappedEvent();
+  // No teardown events are fired because neither the create event nor the tab
+  // ID mapping occurred.
+  ExpectFireOnRemovedEvent(0);
+  ExpectFireOnUnmappedEvent(0);
   ASSERT_HRESULT_SUCCEEDED(bho_with_site_->SetSite(NULL));
 }
 
@@ -573,8 +615,8 @@ TEST_F(BrowserHelperObjectTest, HandleNavigateComplete) {
   ExpectTopBrowserNavigation(false, false);
   bho_->HandleNavigateComplete(browser_, CComBSTR(kUrl1));
 
-  ExpectFireOnRemovedEvent();
-  ExpectFireOnUnmappedEvent();
+  ExpectFireOnRemovedEvent(1);
+  ExpectFireOnUnmappedEvent(0);
   ASSERT_HRESULT_SUCCEEDED(bho_with_site_->SetSite(NULL));
 }
 
@@ -601,8 +643,8 @@ TEST_F(BrowserHelperObjectTest, OnNavigationCompletedWithUnrelatedBrowser) {
           Times(Exactly(0));
 
   bho_->HandleNavigateComplete(browser2, CComBSTR(kUrl1));
-  ExpectFireOnRemovedEvent();
-  ExpectFireOnUnmappedEvent();
+  ExpectFireOnRemovedEvent(0);
+  ExpectFireOnUnmappedEvent(0);
   ASSERT_HRESULT_SUCCEEDED(bho_with_site_->SetSite(NULL));
 }
 
@@ -657,8 +699,10 @@ TEST_F(BrowserHelperObjectTest, AttachOrphanedBrowser) {
   EXPECT_HRESULT_SUCCEEDED(bho_->AttachBrowser(browser3, browser3_parent,
                                                handler3));
 
-  ExpectFireOnRemovedEvent();
-  ExpectFireOnUnmappedEvent();
+  // No teardown events are fired because neither the create event nor the tab
+  // ID mapping occurred.
+  ExpectFireOnRemovedEvent(0);
+  ExpectFireOnUnmappedEvent(0);
   ASSERT_HRESULT_SUCCEEDED(bho_with_site_->SetSite(NULL));
 }
 
@@ -682,13 +726,36 @@ TEST_F(BrowserHelperObjectTest, IFrameEventHandlerHost) {
   // But not twice.
   EXPECT_HRESULT_FAILED(bho_->DetachBrowser(browser_, NULL, handler_));
 
-  ExpectFireOnRemovedEvent();
-  ExpectFireOnUnmappedEvent();
+  ExpectFireOnRemovedEvent(0);
+  ExpectFireOnUnmappedEvent(0);
   ASSERT_HRESULT_SUCCEEDED(bho_with_site_->SetSite(NULL));
 
   // TODO(siggi@chromium.org): test hierarchial attach/detach/TearDown.
 }
 
+TEST_F(BrowserHelperObjectTest, IFrameEventHandlerHostReadyStateChanged) {
+  CreateSite();
+  CreateBrowser();
+  CreateHandler();
+  ExpectChromeFrameGetSessionId();
+
+  ASSERT_HRESULT_SUCCEEDED(bho_with_site_->SetSite(site_keeper_));
+  ASSERT_TRUE(BhoHasSite());
+  ASSERT_HRESULT_SUCCEEDED(bho_->AttachBrowser(browser_, NULL, handler_));
+
+  ExpectHandleNavigation(handler_, true);
+  ExpectTopBrowserNavigation(true, true);
+  browser_->FireOnNavigateComplete(browser_, &CComVariant(kUrl1));
+
+  EXPECT_CALL(bho_->mock_tab_events_funnel_,
+              OnUpdated(_, NULL, READYSTATE_COMPLETE)).Times(1);
+  ASSERT_HRESULT_SUCCEEDED(bho_->OnReadyStateChanged(READYSTATE_LOADING));
+
+  ExpectFireOnRemovedEvent(1);
+  ExpectFireOnUnmappedEvent(1);
+  ASSERT_HRESULT_SUCCEEDED(bho_with_site_->SetSite(NULL));
+}
+
 TEST_F(BrowserHelperObjectTest, InsertCode) {
   CreateSite();
   CreateBrowser();
@@ -706,8 +773,8 @@ TEST_F(BrowserHelperObjectTest, InsertCode) {
   ASSERT_HRESULT_SUCCEEDED(bho_->InsertCode(code, file, FALSE,
                                             kCeeeTabCodeTypeCss));
 
-  ExpectFireOnRemovedEvent();
-  ExpectFireOnUnmappedEvent();
+  ExpectFireOnRemovedEvent(0);
+  ExpectFireOnUnmappedEvent(1);
   ASSERT_HRESULT_SUCCEEDED(bho_with_site_->SetSite(NULL));
 }
 
@@ -746,8 +813,8 @@ TEST_F(BrowserHelperObjectTest, InsertCodeAllFrames) {
   ASSERT_HRESULT_SUCCEEDED(bho_->InsertCode(code, file, TRUE,
                                             kCeeeTabCodeTypeJs));
 
-  ExpectFireOnRemovedEvent();
-  ExpectFireOnUnmappedEvent();
+  ExpectFireOnRemovedEvent(0);
+  ExpectFireOnUnmappedEvent(1);
   ASSERT_HRESULT_SUCCEEDED(bho_with_site_->SetSite(NULL));
 }
 
@@ -781,8 +848,8 @@ TEST_F(BrowserHelperObjectTest, IsHashChange) {
   EXPECT_FALSE(bho_->CallIsHashChange(url3, url6));
   EXPECT_FALSE(bho_->CallIsHashChange(url5, url6));
 
-  ExpectFireOnRemovedEvent();
-  ExpectFireOnUnmappedEvent();
+  ExpectFireOnRemovedEvent(0);
+  ExpectFireOnUnmappedEvent(0);
   ASSERT_HRESULT_SUCCEEDED(bho_with_site_->SetSite(NULL));
 }
 
@@ -799,8 +866,8 @@ TEST_F(BrowserHelperObjectTest, SetToolBandSessionId) {
         kGoodTabHandle)).WillOnce(Return(E_FAIL));
   EXPECT_EQ(E_FAIL, bho_->SetToolBandSessionId(kGoodTabId));
 
-  ExpectFireOnRemovedEvent();
-  ExpectFireOnUnmappedEvent();
+  ExpectFireOnRemovedEvent(0);
+  ExpectFireOnUnmappedEvent(0);
   ASSERT_HRESULT_SUCCEEDED(bho_with_site_->SetSite(NULL));
 }