Have the Debugger extension api check that it has access to the tab

chrome/browser/extensions/api/debugger/debugger_api.cc

Index: chrome/browser/extensions/api/debugger/debugger_api.cc
diff --git a/chrome/browser/extensions/api/debugger/debugger_api.cc b/chrome/browser/extensions/api/debugger/debugger_api.cc
index ebbc10207223aa9f07f5fed2ca7ee92480921f65..8b8df9ff2f70a272e694a409ec389dcc26c6c3ad 100644
--- a/chrome/browser/extensions/api/debugger/debugger_api.cc
+++ b/chrome/browser/extensions/api/debugger/debugger_api.cc
@@ -26,6 +26,7 @@
 #include "chrome/browser/extensions/extension_tab_util.h"
 #include "chrome/browser/infobars/infobar_service.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/sessions/session_id.h"
 #include "chrome/browser/ui/webui/chrome_web_ui_controller_factory.h"
 #include "chrome/common/chrome_switches.h"
 #include "components/infobars/core/confirm_infobar_delegate.h"
@@ -49,6 +50,7 @@
 #include "extensions/browser/extension_system.h"
 #include "extensions/common/error_utils.h"
 #include "extensions/common/extension.h"
+#include "extensions/common/permissions/permissions_data.h"
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 
@@ -588,13 +590,15 @@ bool DebuggerAttachFunction::RunAsync() {
     return false;
   }
 
+  const Extension* extension = GetExtension();
+
   infobars::InfoBar* infobar = NULL;
   if (!CommandLine::ForCurrentProcess()->
        HasSwitch(switches::kSilentDebuggerExtensionAPI)) {
     // Do not attach to the target if for any reason the infobar cannot be shown
     // for this WebContents instance.
     infobar = ExtensionDevToolsInfoBarDelegate::Create(
-        agent_host_->GetRenderViewHost(), GetExtension()->name());
+        agent_host_->GetRenderViewHost(), extension->name());

[not at google - send to devlin, 2014/06/23 19:34:19]

what is the lifetime/behaviour of constructing the infobar here and then later
(with your permission check) not using it?

for safety's sake we might as well do the permission check you're adding before
attempting this.

[Devlin, 2014/06/24 17:49:29]

Done.

     if (!infobar) {
       error_ = ErrorUtils::FormatErrorMessage(
           keys::kSilentDebuggingRequired,
@@ -603,10 +607,28 @@ bool DebuggerAttachFunction::RunAsync() {
     }
   }
 
+  content::WebContents* web_contents =
+      content::WebContents::FromRenderViewHost(
+          agent_host_->GetRenderViewHost());
+  if (!web_contents) {
+    FormatErrorMessage(keys::kNoTargetError);
+    return false;
+  }
+
+  if (!extension->permissions_data()->CanAccessPage(
+          extension,

[not at google - send to devlin, 2014/06/23 19:34:19]

tracing this down, it looks like this is backwards incompatible; extensions
didn't previously need to declare host permissions, now they do.

could you add a special-case for the debugger permission to
PermissionData::CanRunOnPage? we *should* be able to back that change out in
manifest v3 since I can't see anything that allows the API to break site
isolation (e.g. webRequest can break site isolation) but cross that bridge.

[Devlin, 2014/06/24 17:49:29]

I don't really like the idea of giving any extension with the debugger
permission access to all pages, even if we already warned the user about it. 
With the new patch, though, we don't need to.

+          web_contents->GetLastCommittedURL(),
+          web_contents->GetLastCommittedURL(),
+          SessionID::IdForTab(web_contents),
+          -1,  // igore process id.

[meacer, 2014/06/23 19:29:07]

igore -> ignore

[Devlin, 2014/06/24 17:49:29]

Done.

+          &error_)) {
+    return false;

[meacer, 2014/06/23 19:29:07]

You can print an error message here too.

[Devlin, 2014/06/24 17:49:29]

Moot now (but |error_| would have been populated via CanAccessPage()).

+  }
+

[Devlin, 2014/06/23 19:17:47]

Ben, I think you were mentioning that we should also do a check here for where
access to chrome urls could count for chrome-extension urls.  Do you still want
that?

[not at google - send to devlin, 2014/06/23 19:34:19]

I think we should just do that as part of the low-level code which checks the
effect of that flag, but it's a mostly unrelated change (just happens to be
relevant after this change).

   new ExtensionDevToolsClientHost(GetProfile(),
                                   agent_host_.get(),
-                                  GetExtension()->id(),
-                                  GetExtension()->name(),
+                                  extension->id(),
+                                  extension->name(),
                                   debuggee_,
                                   infobar);
   SendResponse(true);