Prevent chrome.windows.create from moving a tab to another profile

chrome/browser/extensions/api/tabs/tabs_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.
 
 #include "chrome/browser/extensions/api/tabs/tabs_api.h"
 
 #include <stddef.h>
 #include <algorithm>
 #include <limits>
 #include <utility>
@@ skipping 441 matching lines @@
       }
       // Don't let the extension crash the browser or renderers.
       if (ExtensionTabUtil::IsKillURL(url)) {
         error_ = keys::kNoCrashBrowserError;
         return false;
       }
       urls.push_back(url);
     }
   }
 
+  // Decide whether we are opening a normal window or an incognito window.
+  bool is_error = true;
+  bool open_incognito_window = ShouldOpenIncognitoWindow(create_data, &urls,
+                                                         &is_error);
+  if (is_error)
+    return false;  // error_ member is set inside of ShouldOpenIncognitoWindow.

[Finnur, 2016/05/17 14:49:14]

nit: This comment doesn't really add anything, I wouldn't retain it.

[Devlin, 2016/05/17 19:29:54]

I disagree a little - it explains why we don't have to set the error_ message
separately here (not to be mistaken with is_error).  If I'm just looking at this
code, I should be on the look out for returning false without having set error_
appropriately.

[Finnur, 2016/05/18 14:06:32]

Oh, error_member! Sorry, I read this as:

// is_error is set inside of ShouldOpenIncognitoWindow.

My bad. Fine to keep it.

+
   // Look for optional tab id.
   if (create_data && create_data->tab_id) {
     // Find the tab. |source_tab_strip| and |tab_index| will later be used to
     // move the tab into the created window.
+    Browser* source_browser = nullptr;

[Devlin, 2016/05/17 19:29:54]

nit: source_browser, to me, implies that this is the browser that sent the
message.  Maybe "tabs_old_browser"?  "move_tab_from_browser"?  Something along
those lines?

[robwu, 2016/05/17 21:14:24]

source as in source of the tab. I'm using source_* to be consistent with
"source_tab_strip". And also with MoveTab.

[Devlin, 2016/05/18 17:09:41]

I still disagree with the naming, but fair point regarding consistency.  This is
fine.

     if (!GetTabById(*create_data->tab_id,
                     GetProfile(),
                     include_incognito(),
-                    NULL,
+                    &source_browser,
                     &source_tab_strip,
-                    NULL,
+                    nullptr,
                     &tab_index,
                     &error_))
       return false;
+
+    if (!source_browser->window()->IsTabStripEditable()) {
+      error_ = keys::kTabStripNotEditableError;
+      return false;
+    }
+
+    bool is_source_incognito =
+        source_browser->profile()->GetProfileType() ==
+        Profile::INCOGNITO_PROFILE;

[Devlin, 2016/05/17 19:29:54]

nit: I think we typically use BrowserContext::IsOffTheRecord() for this check
(so here, just profile()->IsOffTheRecord()).  Is there a reason to prefer
GetProfileType()?

+    if (open_incognito_window != is_source_incognito) {
+      error_ = keys::kCanOnlyMoveTabsWithinSameProfileError;

[Devlin, 2016/05/17 19:29:54]

I also wonder if there's some other tricky edge case we might potentially want
to avoid in the future (I can't think of any yet, but the types of profiles we
have are growing).  Maybe we could just do:
if (window_profile != tabs_profile) {
  // Abort
}
?

It seems like that's really what we want to avoid, so it'd make sense to make
that the direct check.

(That would mean moving window_profile around, but I think that's fine)

[robwu, 2016/05/17 21:14:24]

Done.

+      return false;
+    }
   }
 
   if (!IsValidStateForWindowsCreateFunction(create_data)) {
     error_ = keys::kInvalidWindowStateError;
     return false;
   }
 
   Profile* window_profile = GetProfile();
   Browser::Type window_type = Browser::TYPE_TABBED;
   bool create_panel = false;
 
   // panel_create_mode only applies if create_panel = true
   PanelManager::CreateMode panel_create_mode = PanelManager::CREATE_AS_DOCKED;
 
   gfx::Rect window_bounds;
   bool focused = true;
   bool saw_focus_key = false;
   std::string extension_id;
-
-  // Decide whether we are opening a normal window or an incognito window.
-  bool is_error = true;
-  bool open_incognito_window = ShouldOpenIncognitoWindow(create_data, &urls,
-                                                         &is_error);
-  if (is_error) {
-    // error_ member variable is set inside of ShouldOpenIncognitoWindow.
-    return false;
-  }
   if (open_incognito_window) {
     window_profile = window_profile->GetOffTheRecordProfile();
   }
 
   if (create_data) {
     // Figure out window type before figuring out bounds so that default
     // bounds can be set according to the window type.
     switch (create_data->type) {
       case windows::CREATE_TYPE_POPUP:
         window_type = Browser::TYPE_POPUP;
@@ skipping 1634 matching lines @@
   ZoomModeToZoomSettings(zoom_mode, &zoom_settings);
   zoom_settings.default_zoom_factor.reset(new double(
       content::ZoomLevelToZoomFactor(zoom_controller->GetDefaultZoomLevel())));
 
   results_ = api::tabs::GetZoomSettings::Results::Create(zoom_settings);
   SendResponse(true);
   return true;
 }
 
 }  // namespace extensions