Don't crash when granting permissions to active tab.

Don't crash when granting permissions to active tab.

It seems unlikely for active tab permissions to be granted to a tab that
is closing but on the other hand there is nothing that would prevent a
race condition in which the RenderView is destroyed before the
ExtensionMsg_UpdateTabSpecificPermissions message is processed.

The ExtensionMsg_{Update,Clear}TabSpecificPermissions message is changed
to a ROUTED message so that it is sent to the specific RenderView directly.
This removes the need to find the RenderView by tab_id which could fail.

BUG=396232
Committed: https://crrev.com/8747c1d912e10bbb8fe746ff6a06bb5a3d067efc
Cr-Commit-Position: refs/heads/master@{#304784}

[msimonides, 2014-11-06 10:11:57]

msimonides@opera.com changed reviewers:
+ mek@chromium.org

[msimonides, 2014-11-12 14:32:55]

msimonides@opera.com changed reviewers:
+ kalman@chromium.org

[msimonides, 2014-11-12 14:32:55]

Adding kalman to reviewers, he seems to be the original author of
OnUpdateTabSpecificPermissions in Dispatcher (before it was moved).

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

https://codereview.chromium.org/708713003/diff/1/chrome/renderer/extensions/c...
File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc
(right):

https://codereview.chromium.org/708713003/diff/1/chrome/renderer/extensions/c...
chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:313: // For
now, the message should only be sent to the render view that contains
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]

https://codereview.chromium.org/708713003/diff/1/chrome/renderer/extensions/c...
File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc
(right):

https://codereview.chromium.org/708713003/diff/1/chrome/renderer/extensions/c...
chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:313: // For
now, the message should only be sent to the render view that contains
On 2014/11/12 16:28:09, kalman wrote:
> 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.

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?

> [...]
> 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.

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:38]

https://codereview.chromium.org/708713003/diff/1/chrome/renderer/extensions/c...
File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc
(right):

https://codereview.chromium.org/708713003/diff/1/chrome/renderer/extensions/c...
chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:313: // For
now, the message should only be sent to the render view that contains
On 2014/11/13 17:13:20, msimonides wrote:
> On 2014/11/12 16:28:09, kalman wrote:
> > 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.
> 
> 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?

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.

> 
> > [...]
> > 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.
> 
> Makes sense, I'll go for the latter solution. Should I also change
> ExtensionMsg_ClearTabSpecificPermissions to be a routed message?

That makes sense, yeah.

[msimonides, 2014-11-14 08:51:09]

msimonides@opera.com changed reviewers:
+ palmer@chromium.org

[palmer, 2014-11-14 19:14:12]

IPC security LGTM.

[msimonides, 2014-11-18 07:10:18]

ping!

[not at google - send to devlin, 2014-11-18 15:44:41]

lgtm, this patch is awesome.

https://codereview.chromium.org/708713003/diff/20001/extensions/renderer/exte...
File extensions/renderer/extension_helper.cc (right):

https://codereview.chromium.org/708713003/diff/20001/extensions/renderer/exte...
extensions/renderer/extension_helper.cc:297: for
(std::vector<std::string>::const_iterator it = extension_ids.begin();
Convert this to foreach syntax while you're here?

[msimonides, 2014-11-19 07:51:06]

The CQ bit was checked by msimonides@opera.com

[commit-bot: I haz the power, 2014-11-19 07:52:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/708713003/40001

[commit-bot: I haz the power, 2014-11-19 08:32:38]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-11-19 08:33:36]

Patchset 3 (id:??) landed as
https://crrev.com/8747c1d912e10bbb8fe746ff6a06bb5a3d067efc
Cr-Commit-Position: refs/heads/master@{#304784}