Don't crash when granting permissions to active tab.

chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc

 // Copyright 2014 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/renderer/extensions/chrome_extensions_dispatcher_delegate.h"
 
 #include "base/command_line.h"
 #include "base/sha1.h"
 #include "base/strings/string_number_conversions.h"
 #include "chrome/common/chrome_switches.h"
@@ skipping 289 matching lines @@
   }
 }
 
 void ChromeExtensionsDispatcherDelegate::UpdateTabSpecificPermissions(
     const extensions::Dispatcher* dispatcher,
     const GURL& url,
     int tab_id,
     const std::string& extension_id,
     const extensions::URLPatternSet& origin_set) {
   content::RenderView* view = extensions::TabFinder::Find(tab_id);
+  if (!view)
+    return;
 
   // For now, the message should only be sent to the render view that contains

[not at google - send to devlin, 2014/11/12 16:28:09]

Was this actually crashing before? The comment indicates that it's not possible
for view to be NULL because it was the view that the message was sent to. Which
makes sense.

In that case it actually looks like the code is wrong. Sending to a tab ID
doesn't make sense since you know the RenderView that it was delivered to,
implying that ExtensionMsg_UpdateTabSpecificPermissions should be a ROUTED IPC,
delivered to an ExtensionHelper, and pass through the RenderView.

I guess the point of the comment (and the existing code?) is that we might want
to notify *all* renderers of the new permissions, not just the affected
renderers, but I think that's unlikely.

To summarise:
- Your change make sense insofar as the existing code doesn't make sense, and
your change demonstrates that.
- A better fix would be to pass the RenderView into this method rather than the
tab ID, meaning making ExtensionMsg_UpdateTabSpecificPermissions ROUTED and
delivering it to the ExtensionHelper rather than Dispatcher.

[msimonides, 2014/11/13 17:13:20]

Unfortunately I'm unable to reproduce the crash, we only see it reported for
Opera in the wild, so I can only speculate why view is NULL here.
BTW: does Chrome/Chromium get reports of a crash in this function?
Makes sense, I'll go for the latter solution. Should I also change
ExtensionMsg_ClearTabSpecificPermissions to be a routed message?

[not at google - send to devlin, 2014/11/13 18:17:37]

Good question. I'm seeing crashes on line 312, GURL
active_url(view->GetWebView()->mainFrame()->document().url()), and that may be
because view is NULL.
That makes sense, yeah.

   // the target tab. This may change. Either way, if this is the target tab it
   // gives us the chance to check against the URL to avoid races.
-  DCHECK(view);
   GURL active_url(view->GetWebView()->mainFrame()->document().url());
   if (active_url != url)
     return;
 
   const extensions::Extension* extension =
       dispatcher->extensions()->GetByID(extension_id);
   if (!extension)
     return;
 
   extension->permissions_data()->UpdateTabSpecificPermissions(
       tab_id,
       new extensions::PermissionSet(extensions::APIPermissionSet(),
                                     extensions::ManifestPermissionSet(),
                                     origin_set,
                                     extensions::URLPatternSet()));
 }