Don't send unnecessary ExtensionMsg_Loaded IPCs.

Don't send unnecessary ExtensionMsg_Loaded IPCs.

Each time ExtensionMsg_Loaded is sent it re-creates the set of Extensions it
gives, invalidating the existing ones. This means that anything that holds onto
Extensions for long periods of time - like ScriptContextSet - can end up in
situations where they have an Extension with the same ID as an installed
Extension, but different in pointer equality.

It turns out that we send ExtensionMsg_Loaded every time a RenderViewHost is
created, but it should only be send every time a RenderProcessHost is created,
because the Extension sets are process-wide.

While this bug is fundamental, one way it manifests itself is by the
chrome.permissions.request API not updating the JavaScript context when
permissions are granted.

BUG=435141
Committed: https://crrev.com/8bcbc759e797556b4bd4ec80d22e597a8ca12695
Cr-Commit-Position: refs/heads/master@{#332720}

[not at google - send to devlin, 2015-06-03 00:46:58]

Patchset #3 (id:40001) has been deleted

[not at google - send to devlin, 2015-06-03 20:26:33]

kalman@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Devlin, 2015-06-03 20:53:37]

https://codereview.chromium.org/1142993002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/permissions/permissions_apitest.cc (right):

https://codereview.chromium.org/1142993002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/permissions/permissions_apitest.cc:200: //
(chrome.whatever, in this case chrome.storage). Regression test for
The test seems to use chrome.alarms?

https://codereview.chromium.org/1142993002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/permissions/permissions_apitest.cc:201: //
crbug.com/435141, see details there for trickyness.
s/trickyness/trickiness

https://codereview.chromium.org/1142993002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/permissions/permissions_apitest.cc:203:
ASSERT_TRUE(RunExtensionTest("permissions/optional_updates_bindings"))
I trust this test failed without this patch?

https://codereview.chromium.org/1142993002/diff/60001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/permissions/optional_updates_bindings/background.js
(right):

https://codereview.chromium.org/1142993002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/permissions/optional_updates_bindings/background.js:17:
Object.keys(w.chrome.alarms).sort());
While the alarms api is pretty stable, I think it'd still be the right-er thing
to do to make sure that we contain at least those keys, versus strict equality.

https://codereview.chromium.org/1142993002/diff/60001/extensions/browser/exte...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1142993002/diff/60001/extensions/browser/exte...
extensions/browser/extension_web_contents_observer.cc:97: // Note that because
this code is run per *view* we may end up sending
I think more important is that the Dispatcher is keyed to the RenderThread,
which can (AFAIK) outlive views, but we never de-activate an extension.  So any
number of extensions could be active.

That said, I think we should also look into removing this ActivateExtension
thingy.  It's weird, and not used very much.

https://codereview.chromium.org/1142993002/diff/60001/extensions/renderer/dis...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1142993002/diff/60001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:863: // This test is deliberately not a CHECK
(though I wish it could be) and
This kind of sounds like a TODO.  Wanna own it? :)

https://codereview.chromium.org/1142993002/diff/60001/extensions/renderer/dis...
File extensions/renderer/dispatcher.h (right):

https://codereview.chromium.org/1142993002/diff/60001/extensions/renderer/dis...
extensions/renderer/dispatcher.h:141: // to call InsertExtensionForTest and the
OnActivateExtension IPCs.
There is no InsertExtensionForTest ;)

[not at google - send to devlin, 2015-06-03 21:53:03]

https://codereview.chromium.org/1142993002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/permissions/permissions_apitest.cc (right):

https://codereview.chromium.org/1142993002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/permissions/permissions_apitest.cc:200: //
(chrome.whatever, in this case chrome.storage). Regression test for
On 2015/06/03 20:53:36, Devlin wrote:
> The test seems to use chrome.alarms?

Done.

https://codereview.chromium.org/1142993002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/permissions/permissions_apitest.cc:201: //
crbug.com/435141, see details there for trickyness.
On 2015/06/03 20:53:37, Devlin wrote:
> s/trickyness/trickiness

Done.

https://codereview.chromium.org/1142993002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/permissions/permissions_apitest.cc:203:
ASSERT_TRUE(RunExtensionTest("permissions/optional_updates_bindings"))
On 2015/06/03 20:53:37, Devlin wrote:
> I trust this test failed without this patch?

...

https://codereview.chromium.org/1142993002/diff/60001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/permissions/optional_updates_bindings/background.js
(right):

https://codereview.chromium.org/1142993002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/permissions/optional_updates_bindings/background.js:17:
Object.keys(w.chrome.alarms).sort());
On 2015/06/03 20:53:37, Devlin wrote:
> While the alarms api is pretty stable, I think it'd still be the right-er
thing
> to do to make sure that we contain at least those keys, versus strict
equality.

I think it's stronger to check all, and I don't see why not. I picked alarms
specifically because it's almost certainly not going to change, but if somebody
does decide to add something for some reason, we can update it then.

https://codereview.chromium.org/1142993002/diff/60001/extensions/browser/exte...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1142993002/diff/60001/extensions/browser/exte...
extensions/browser/extension_web_contents_observer.cc:97: // Note that because
this code is run per *view* we may end up sending
> I think more important is that the Dispatcher is keyed to the RenderThread,
> which can (AFAIK) outlive views, but we never de-activate an extension.  So
any
> number of extensions could be active.

The comment is about the fact that extension activation can only happen on
render process (host) creation, so we'd potentially save some IPCs by doing it
there.

It's less related about the *other* weirdness where this activated extension set
is stored process-wide rather than on the process abstraction level. Unless
RenderThread is a process abstraction...? I don't know. Anyway I'll update the
comment to make it more obvious that it's about saving IPCs.

> 
> That said, I think we should also look into removing this ActivateExtension
> thingy.  It's weird, and not used very much.

It's important at the moment, but it won't be necessary with site isolation.
That will be a good time to delete it.

https://codereview.chromium.org/1142993002/diff/60001/extensions/renderer/dis...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1142993002/diff/60001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:863: // This test is deliberately not a CHECK
(though I wish it could be) and
On 2015/06/03 20:53:37, Devlin wrote:
> This kind of sounds like a TODO.  Wanna own it? :)

well I don't plan on doing it, but sure :-)

https://codereview.chromium.org/1142993002/diff/60001/extensions/renderer/dis...
File extensions/renderer/dispatcher.h (right):

https://codereview.chromium.org/1142993002/diff/60001/extensions/renderer/dis...
extensions/renderer/dispatcher.h:141: // to call InsertExtensionForTest and the
OnActivateExtension IPCs.
On 2015/06/03 20:53:37, Devlin wrote:
> There is no InsertExtensionForTest ;)

LoadExtensionForTest, oops.

[Devlin, 2015-06-03 21:56:44]

lgtm

https://codereview.chromium.org/1142993002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/permissions/permissions_apitest.cc (right):

https://codereview.chromium.org/1142993002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/permissions/permissions_apitest.cc:203:
ASSERT_TRUE(RunExtensionTest("permissions/optional_updates_bindings"))
On 2015/06/03 21:53:03, kalman wrote:
> On 2015/06/03 20:53:37, Devlin wrote:
> > I trust this test failed without this patch?
> 
> ...

Yeah, okay, that was silly.  I trust this test fails if we sent redundant IPCs,
as we did before this patch?

[not at google - send to devlin, 2015-06-03 22:05:36]

https://codereview.chromium.org/1142993002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/permissions/permissions_apitest.cc (right):

https://codereview.chromium.org/1142993002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/permissions/permissions_apitest.cc:203:
ASSERT_TRUE(RunExtensionTest("permissions/optional_updates_bindings"))
On 2015/06/03 21:56:44, Devlin wrote:
> On 2015/06/03 21:53:03, kalman wrote:
> > On 2015/06/03 20:53:37, Devlin wrote:
> > > I trust this test failed without this patch?
> > 
> > ...
> 
> Yeah, okay, that was silly.  I trust this test fails if we sent redundant
IPCs,
> as we did before this patch?

I'm not really sure how to test that without more or less going back to the
state before this patch :)

[not at google - send to devlin, 2015-06-03 22:05:40]

The CQ bit was checked by kalman@chromium.org

[commit-bot: I haz the power, 2015-06-03 22:07:23]

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

[Devlin, 2015-06-03 22:17:49]

https://codereview.chromium.org/1142993002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/permissions/permissions_apitest.cc (right):

https://codereview.chromium.org/1142993002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/permissions/permissions_apitest.cc:203:
ASSERT_TRUE(RunExtensionTest("permissions/optional_updates_bindings"))
On 2015/06/03 22:05:36, kalman wrote:
> On 2015/06/03 21:56:44, Devlin wrote:
> > On 2015/06/03 21:53:03, kalman wrote:
> > > On 2015/06/03 20:53:37, Devlin wrote:
> > > > I trust this test failed without this patch?
> > > 
> > > ...
> > 
> > Yeah, okay, that was silly.  I trust this test fails if we sent redundant
> IPCs,
> > as we did before this patch?
> 
> I'm not really sure how to test that without more or less going back to the
> state before this patch :)

You say that like it's hard. :P
git checkout origin -b test_api_regression
git checkout <whatever this branch is>
chrome/test/data/extensions/api_test/permissions/optional_updates_bindings
git checkout <whatever this branch is>
chrome/browser/extensions/api/permissions/permissions_apitest.cc

Done.

For the record, this does indeed fail. :)

(I only ask because if you say it's a regression test, it's good to make sure it
would have otherwise detected the failure.)

[commit-bot: I haz the power, 2015-06-03 23:12:36]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-06-03 23:13:33]

Patchset 4 (id:??) landed as
https://crrev.com/8bcbc759e797556b4bd4ec80d22e597a8ca12695
Cr-Commit-Position: refs/heads/master@{#332720}