We must return an error if we fail to validate a tab while trying to validate it's parent frame.

BUG=None
TEST=None

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

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

2 files changed, 80 insertions, 55 deletions

diff --git a/ceee/ie/broker/tab_api_module.cc b/ceee/ie/broker/tab_api_module.cc
index 47a128f..b50b5ff 100644
--- a/ceee/ie/broker/tab_api_module.cc
+++ b/ceee/ie/broker/tab_api_module.cc
@@ -20,12 +20,13 @@
 #include "base/json/json_reader.h"
 #include "base/json/json_writer.h"
 #include "base/logging.h"
-#include "base/scoped_comptr_win.h"
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
 #include "base/win/scoped_bstr.h"
+#include "base/win/scoped_comptr.h"
+#include "base/win/scoped_variant.h"
 #include "base/win/windows_version.h"
 #include "ceee/ie/broker/api_module_constants.h"
 #include "ceee/ie/broker/api_module_util.h"
@@ -174,7 +175,7 @@ bool TabApiResult::CreateTabValue(int tab_id, long index) {
     return false;
   }
 
-  ScopedComPtr<ICeeeTabExecutor> executor;
+  base::win::ScopedComPtr<ICeeeTabExecutor> executor;
   dispatcher->GetExecutor(tab_window, IID_ICeeeTabExecutor,
                           reinterpret_cast<void**>(executor.Receive()));
   if (executor == NULL) {
@@ -238,7 +239,7 @@ bool TabApiResult::CreateTabValue(int tab_id, long index) {
   // so we can save an IPC call.
   if (index == -1) {
     // We need another executor to get the index from the frame window thread.
-    ScopedComPtr<ICeeeWindowExecutor> executor;
+    base::win::ScopedComPtr<ICeeeWindowExecutor> executor;
     dispatcher->GetExecutor(frame_window, IID_ICeeeWindowExecutor,
                             reinterpret_cast<void**>(executor.Receive()));
     if (executor == NULL) {
@@ -270,7 +271,7 @@ bool TabApiResult::CreateTabValue(int tab_id, long index) {
   return true;
 }
 
-bool TabApiResult::IsTabFromSameOrUnspecifiedFrameWindow(
+HRESULT TabApiResult::IsTabFromSameOrUnspecifiedFrameWindow(
     const DictionaryValue& input_dict,
     const Value* saved_window_value,
     HWND* tab_window,
@@ -279,12 +280,17 @@ bool TabApiResult::IsTabFromSameOrUnspecifiedFrameWindow(
   bool success = input_dict.GetInteger(ext::kIdKey, &tab_id);
   DCHECK(success && tab_id != 0) << "The input_dict MUST have a tab ID!!!";
   DCHECK(dispatcher != NULL);
+  if (!dispatcher->IsTabIdValid(tab_id)) {
+    // This can happen if the tab died before we get here.
+    LOG(WARNING) << "Ta ID: " << tab_id << ", not recognized.";
+    return E_UNEXPECTED;
+  }
   HWND input_tab_window = dispatcher->GetTabHandleFromId(tab_id);
   if (tab_window != NULL)
     *tab_window = input_tab_window;
 
   if (saved_window_value == NULL)
-    return true;
+    return S_OK;
 
   DCHECK(saved_window_value->IsType(Value::TYPE_INTEGER));
   int saved_window_id = 0;
@@ -303,7 +309,7 @@ bool TabApiResult::IsTabFromSameOrUnspecifiedFrameWindow(
     DCHECK_EQ(window_utils::GetTopLevelParent(input_tab_window), frame_window);
   }
 
-  return frame_window_id == saved_window_id;
+  return frame_window_id == saved_window_id ? S_OK : S_FALSE;
 }
 
 bool GetIntegerFromValue(
@@ -507,8 +513,15 @@ HRESULT GetSelectedTab::ContinueExecution(
   }
 
   HWND tab_window = NULL;
-  if (!TabApiResult::IsTabFromSameOrUnspecifiedFrameWindow(*input_dict,
-      result->GetValue(ext::kWindowIdKey), &tab_window, dispatcher)) {
+  HRESULT hr = TabApiResult::IsTabFromSameOrUnspecifiedFrameWindow(
+      *input_dict, result->GetValue(ext::kWindowIdKey), &tab_window,
+      dispatcher);
+  if (FAILED(hr)) {
+    // The ApiDispatcher will forget about us and result's destructor will
+    // free the allocation of user_data.
+    return hr;
+  }
+  if (hr == S_FALSE) {
     // These are not the droids you are looking for. :-)
     result.release();  // The ApiDispatcher will keep it alive.
     return S_FALSE;
@@ -578,7 +591,7 @@ void GetAllTabsInWindow::Execute(const ListValue& args, int request_id) {
 
   ApiDispatcher* dispatcher = GetDispatcher();
   DCHECK(dispatcher != NULL);
-  ScopedComPtr<ICeeeWindowExecutor> executor;
+  base::win::ScopedComPtr<ICeeeWindowExecutor> executor;
   dispatcher->GetExecutor(frame_window, IID_ICeeeWindowExecutor,
                           reinterpret_cast<void**>(executor.Receive()));
   if (executor == NULL) {
@@ -646,7 +659,7 @@ void UpdateTab::Execute(const ListValue& args, int request_id) {
       return;
     }
 
-    ScopedComPtr<ICeeeTabExecutor> executor;
+    base::win::ScopedComPtr<ICeeeTabExecutor> executor;
     dispatcher->GetExecutor(tab_window, IID_ICeeeTabExecutor,
                             reinterpret_cast<void**>(executor.Receive()));
     if (executor == NULL) {
@@ -676,7 +689,7 @@ void UpdateTab::Execute(const ListValue& args, int request_id) {
     // We only take action if the user wants to select the tab; this function
     // does not actually let you deselect a tab.
     if (selected) {
-      ScopedComPtr<ICeeeWindowExecutor> frame_executor;
+      base::win::ScopedComPtr<ICeeeWindowExecutor> frame_executor;
       dispatcher->GetExecutor(
           window_utils::GetTopLevelParent(tab_window), IID_ICeeeWindowExecutor,
           reinterpret_cast<void**>(frame_executor.Receive()));
@@ -726,7 +739,7 @@ void RemoveTab::Execute(const ListValue& args, int request_id) {
     return;
   }
 
-  ScopedComPtr<ICeeeWindowExecutor> frame_executor;
+  base::win::ScopedComPtr<ICeeeWindowExecutor> frame_executor;
   dispatcher->GetExecutor(window_utils::GetTopLevelParent(tab_window),
                           IID_ICeeeWindowExecutor,
                           reinterpret_cast<void**>(frame_executor.Receive()));
@@ -860,7 +873,7 @@ void CreateTab::Execute(const ListValue& args, int request_id) {
   // So bb2284073 & bb2492252 might still occur there.
   std::wstring url_wstring = UTF8ToWide(url_string);
   if (base::win::GetVersion() < base::win::VERSION_VISTA) {
-    ScopedComPtr<IWebBrowser2> browser;
+    base::win::ScopedComPtr<IWebBrowser2> browser;
     HRESULT hr = ie_util::GetWebBrowserForTopLevelIeHwnd(
         frame_window, NULL, browser.Receive());
     DCHECK(SUCCEEDED(hr)) << "Can't get the browser for window: " <<
@@ -871,11 +884,12 @@ void CreateTab::Execute(const ListValue& args, int request_id) {
     }
 
     long flags = selected ? navOpenInNewTab : navOpenInBackgroundTab;
-    hr = browser->Navigate(base::win::ScopedBstr(url_wstring.c_str()),
-                           &CComVariant(flags),
-                           &CComVariant(L"_blank"),
-                           &CComVariant(),  // Post data
-                           &CComVariant());  // Headers
+    hr = browser->Navigate(
+        base::win::ScopedBstr(url_wstring.c_str()),
+        const_cast<VARIANT*>(&base::win::ScopedVariant(flags)),
+        const_cast<VARIANT*>(&base::win::ScopedVariant(L"_blank")),
+        const_cast<VARIANT*>(&base::win::ScopedVariant()),  // Post data
+        const_cast<VARIANT*>(&base::win::ScopedVariant()));  // Headers
     DCHECK(SUCCEEDED(hr)) << "Failed to create tab. " << com::LogHr(hr);
     if (FAILED(hr)) {
       result->PostError("Internal error while trying to create tab.");
@@ -892,7 +906,7 @@ void CreateTab::Execute(const ListValue& args, int request_id) {
       return;
     }
 
-    ScopedComPtr<ICeeeTabExecutor> executor;
+    base::win::ScopedComPtr<ICeeeTabExecutor> executor;
     dispatcher->GetExecutor(existing_tab, IID_ICeeeTabExecutor,
                             reinterpret_cast<void**>(executor.Receive()));
     if (executor == NULL) {
@@ -940,8 +954,15 @@ HRESULT CreateTab::ContinueExecution(const std::string& input_args,
   }
 
   HWND tab_window = NULL;
-  if (!TabApiResult::IsTabFromSameOrUnspecifiedFrameWindow(*input_dict,
-      result->GetValue(ext::kWindowIdKey), &tab_window, dispatcher)) {
+  HRESULT hr = TabApiResult::IsTabFromSameOrUnspecifiedFrameWindow(
+      *input_dict, result->GetValue(ext::kWindowIdKey), &tab_window,
+      dispatcher);
+  if (FAILED(hr)) {
+    // The ApiDispatcher will forget about us and result's destructor will
+    // free the allocation of user_data.
+    return hr;
+  }
+  if (hr == S_FALSE) {
     // These are not the droids you are looking for. :-)
     result.release();  // The ApiDispatcher will keep it alive.
     return S_FALSE;
@@ -980,7 +1001,7 @@ HRESULT CreateTab::ContinueExecution(const std::string& input_args,
     DCHECK(success) << "index_value->GetAsInteger()";
 
     HWND frame_window = window_utils::GetTopLevelParent(tab_window);
-    ScopedComPtr<ICeeeWindowExecutor> frame_executor;
+    base::win::ScopedComPtr<ICeeeWindowExecutor> frame_executor;
     dispatcher->GetExecutor(frame_window, __uuidof(ICeeeWindowExecutor),
                             reinterpret_cast<void**>(frame_executor.Receive()));
     if (frame_executor == NULL) {
@@ -1105,7 +1126,7 @@ void MoveTab::Execute(const ListValue& args, int request_id) {
 
   HWND frame_window = window_utils::GetTopLevelParent(tab_window);
 
-  ScopedComPtr<ICeeeWindowExecutor> frame_executor;
+  base::win::ScopedComPtr<ICeeeWindowExecutor> frame_executor;
   dispatcher->GetExecutor(frame_window, IID_ICeeeWindowExecutor,
                           reinterpret_cast<void**>(frame_executor.Receive()));
   if (frame_executor == NULL) {
@@ -1184,7 +1205,7 @@ ApiDispatcher::InvocationResult* TabsInsertCode::ExecuteImpl(
     return NULL;
   }
 
-  ScopedComPtr<ICeeeTabExecutor> tab_executor;
+  base::win::ScopedComPtr<ICeeeTabExecutor> tab_executor;
   dispatcher->GetExecutor(tab_window, IID_ICeeeTabExecutor,
                           reinterpret_cast<void**>(tab_executor.Receive()));
   if (tab_executor == NULL) {
@@ -1193,10 +1214,9 @@ ApiDispatcher::InvocationResult* TabsInsertCode::ExecuteImpl(
     return NULL;
   }
 
-  *hr = tab_executor->InsertCode(CComBSTR(code.c_str()),
-                                 CComBSTR(file.c_str()),
-                                 all_frames,
-                                 type);
+  *hr = tab_executor->InsertCode(
+      base::win::ScopedBstr(ASCIIToWide(code).c_str()),
+      base::win::ScopedBstr(ASCIIToWide(file).c_str()), all_frames, type);
   return result.release();
 }
 
diff --git a/ceee/ie/broker/tab_api_module_unittest.cc b/ceee/ie/broker/tab_api_module_unittest.cc
index 7181846..b50bb03 100644
--- a/ceee/ie/broker/tab_api_module_unittest.cc
+++ b/ceee/ie/broker/tab_api_module_unittest.cc
@@ -1061,7 +1061,7 @@ MOCK_STATIC_CLASS_BEGIN(MockStaticTabApiResult)
     MOCK_STATIC_INIT2(TabApiResult::IsTabFromSameOrUnspecifiedFrameWindow,
                       IsTabFromSameOrUnspecifiedFrameWindow);
   MOCK_STATIC_INIT_END()
-  MOCK_STATIC4(bool, , IsTabFromSameOrUnspecifiedFrameWindow,
+  MOCK_STATIC4(HRESULT, , IsTabFromSameOrUnspecifiedFrameWindow,
                const DictionaryValue&, const Value*, HWND*, ApiDispatcher*);
 MOCK_STATIC_CLASS_END(MockStaticTabApiResult)
 
@@ -1182,12 +1182,12 @@ TEST_F(TabApiTests, CreateTabContinueExecution) {
   HRESULT hr = invocation.CallContinueExecution("");
   EXPECT_HRESULT_FAILED(hr);
 
-  // IsTabFromSameOrUnspecifiedFrameWindow returns false.
+  // IsTabFromSameOrUnspecifiedFrameWindow returns S_FALSE.
   invocation.AllocateNewResult(kRequestId);
   StrictMock<MockStaticTabApiResult> tab_api_result;
   EXPECT_CALL(tab_api_result,
               IsTabFromSameOrUnspecifiedFrameWindow(_, _, _, NotNull())).
-      WillOnce(Return(false));
+      WillOnce(Return(S_FALSE));
 
   hr = invocation.CallContinueExecution(input_args);
   EXPECT_HRESULT_SUCCEEDED(hr);
@@ -1199,7 +1199,7 @@ TEST_F(TabApiTests, CreateTabContinueExecution) {
 
   EXPECT_CALL(tab_api_result,
               IsTabFromSameOrUnspecifiedFrameWindow(_, _, _, NotNull())).
-      WillOnce(Return(true));
+      WillOnce(Return(S_OK));
 
   hr = invocation.CallContinueExecution(input_args);
   EXPECT_HRESULT_SUCCEEDED(hr);
@@ -1211,7 +1211,7 @@ TEST_F(TabApiTests, CreateTabContinueExecution) {
 
   EXPECT_CALL(tab_api_result,
               IsTabFromSameOrUnspecifiedFrameWindow(_, _, _, NotNull())).
-      WillOnce(DoAll(SetArgumentPointee<2>(kGoodTabWindow), Return(true)));
+      WillOnce(DoAll(SetArgumentPointee<2>(kGoodTabWindow), Return(S_OK)));
   EXPECT_CALL(invocation.mock_api_dispatcher_,
       GetTabIdFromHandle(kGoodTabWindow)).WillOnce(Return(kGoodTabWindowId));
 
@@ -1231,7 +1231,7 @@ TEST_F(TabApiTests, CreateTabContinueExecution) {
 
   EXPECT_CALL(tab_api_result,
               IsTabFromSameOrUnspecifiedFrameWindow(_, _, _, NotNull())).
-      WillOnce(DoAll(SetArgumentPointee<2>(kGoodTabWindow), Return(true)));
+      WillOnce(DoAll(SetArgumentPointee<2>(kGoodTabWindow), Return(S_OK)));
   EXPECT_CALL(invocation.mock_api_dispatcher_,
       GetTabIdFromHandle(kGoodTabWindow)).WillOnce(Return(kGoodTabWindowId));
 
@@ -1498,24 +1498,27 @@ TEST_F(TabApiTests, IsTabFromSameOrUnspecifiedFrameWindow) {
   // We need a mock for this now.
   StrictMock<MockApiDispatcher> mock_api_dispatcher;
 
-  // We expect these calls repeatedly
+  // We expect these calls repeatedly.
+  // TODO(mad@chromium): Add test cases for when they return other values.
   EXPECT_CALL(mock_api_dispatcher, GetTabHandleFromId(kGoodTabWindowId)).
       WillRepeatedly(Return(kGoodTabWindow));
   EXPECT_CALL(mock_api_dispatcher, GetWindowIdFromHandle(kGoodFrameWindow)).
       WillRepeatedly(Return(kGoodFrameWindowId));
   EXPECT_CALL(mock_api_dispatcher, GetWindowHandleFromId(kGoodFrameWindowId)).
       WillRepeatedly(Return(kGoodFrameWindow));
+  EXPECT_CALL(mock_api_dispatcher, IsTabIdValid(kGoodTabWindowId)).
+      WillRepeatedly(Return(true));
 
   // We always need a kIdKey value in the input_dict.
   DictionaryValue input_dict;
   input_dict.SetInteger(ext::kIdKey, kGoodTabWindowId);
   // Start with no saved dict, so any input value is good.
-  EXPECT_TRUE(TabApiResult::IsTabFromSameOrUnspecifiedFrameWindow(
-      input_dict, NULL, NULL, &mock_api_dispatcher));
+  EXPECT_EQ(TabApiResult::IsTabFromSameOrUnspecifiedFrameWindow(
+      input_dict, NULL, NULL, &mock_api_dispatcher), S_OK);
   // Also test that we are properly returned the input value.
   HWND tab_window = NULL;
-  EXPECT_TRUE(TabApiResult::IsTabFromSameOrUnspecifiedFrameWindow(
-      input_dict, NULL, &tab_window, &mock_api_dispatcher));
+  EXPECT_EQ(TabApiResult::IsTabFromSameOrUnspecifiedFrameWindow(
+      input_dict, NULL, &tab_window, &mock_api_dispatcher), S_OK);
   EXPECT_EQ(kGoodTabWindow, tab_window);
 
   // Now check with the same value found as a grand parent.
@@ -1523,37 +1526,39 @@ TEST_F(TabApiTests, IsTabFromSameOrUnspecifiedFrameWindow) {
   StrictMock<testing::MockWindowUtils> window_utils;
   EXPECT_CALL(window_utils, GetTopLevelParent(kGoodTabWindow)).
       WillRepeatedly(Return(kGoodFrameWindow));
-  EXPECT_TRUE(TabApiResult::IsTabFromSameOrUnspecifiedFrameWindow(
-      input_dict, &saved_window, NULL, &mock_api_dispatcher));
+  EXPECT_EQ(TabApiResult::IsTabFromSameOrUnspecifiedFrameWindow(
+      input_dict, &saved_window, NULL, &mock_api_dispatcher), S_OK);
   tab_window = NULL;
-  EXPECT_TRUE(TabApiResult::IsTabFromSameOrUnspecifiedFrameWindow(
-      input_dict, &saved_window, &tab_window, &mock_api_dispatcher));
+  EXPECT_EQ(TabApiResult::IsTabFromSameOrUnspecifiedFrameWindow(
+      input_dict, &saved_window, &tab_window, &mock_api_dispatcher), S_OK);
   EXPECT_EQ(kGoodTabWindow, tab_window);
 
   // Now check with the same value provided in the input_dict.
   input_dict.SetInteger(ext::kWindowIdKey, kGoodFrameWindowId);
-  EXPECT_TRUE(TabApiResult::IsTabFromSameOrUnspecifiedFrameWindow(
-      input_dict, &saved_window, NULL, &mock_api_dispatcher));
+  EXPECT_EQ(TabApiResult::IsTabFromSameOrUnspecifiedFrameWindow(
+      input_dict, &saved_window, NULL, &mock_api_dispatcher), S_OK);
   tab_window = NULL;
-  EXPECT_TRUE(TabApiResult::IsTabFromSameOrUnspecifiedFrameWindow(
-      input_dict, &saved_window, &tab_window, &mock_api_dispatcher));
+  EXPECT_EQ(TabApiResult::IsTabFromSameOrUnspecifiedFrameWindow(
+      input_dict, &saved_window, &tab_window, &mock_api_dispatcher), S_OK);
   EXPECT_EQ(kGoodTabWindow, tab_window);
 
   // And now check the cases where they differ.
   FundamentalValue other_saved_window(kGoodFrameWindowId + 1);
-  EXPECT_FALSE(TabApiResult::IsTabFromSameOrUnspecifiedFrameWindow(
-      input_dict, &other_saved_window, NULL, &mock_api_dispatcher));
+  EXPECT_EQ(TabApiResult::IsTabFromSameOrUnspecifiedFrameWindow(
+      input_dict, &other_saved_window, NULL, &mock_api_dispatcher), S_FALSE);
   tab_window = NULL;
-  EXPECT_FALSE(TabApiResult::IsTabFromSameOrUnspecifiedFrameWindow(
-      input_dict, &other_saved_window, &tab_window, &mock_api_dispatcher));
+  EXPECT_EQ(TabApiResult::IsTabFromSameOrUnspecifiedFrameWindow(
+      input_dict, &other_saved_window, &tab_window, &mock_api_dispatcher),
+          S_FALSE);
   EXPECT_EQ(kGoodTabWindow, tab_window);
 
   input_dict.Remove(ext::kWindowIdKey, NULL);
-  EXPECT_FALSE(TabApiResult::IsTabFromSameOrUnspecifiedFrameWindow(
-      input_dict, &other_saved_window, NULL, &mock_api_dispatcher));
+  EXPECT_EQ(TabApiResult::IsTabFromSameOrUnspecifiedFrameWindow(
+      input_dict, &other_saved_window, NULL, &mock_api_dispatcher), S_FALSE);
   tab_window = NULL;
-  EXPECT_FALSE(TabApiResult::IsTabFromSameOrUnspecifiedFrameWindow(
-      input_dict, &other_saved_window, &tab_window, &mock_api_dispatcher));
+  EXPECT_EQ(TabApiResult::IsTabFromSameOrUnspecifiedFrameWindow(
+      input_dict, &other_saved_window, &tab_window, &mock_api_dispatcher),
+          S_FALSE);
   EXPECT_EQ(kGoodTabWindow, tab_window);
 }