[Extensions] Don't run unload listeners when an extension is removed

[Extensions] Don't run unload listeners when an extension is removed

When an extension is removed, we close all the tabs for that extension*.
If any of these tabs had unload or beforeunload listeners, this close
process is asynchronous. However, if the listener performs any network
request, this would result in Chrome killing the tab for having an
invalid origin (the extension has been removed).

Instead, don't run these listeners at all. Since the extension has been
removed, there's no reason the code in the tab should run - the
extension should not be able to continue acting.

*a tab for an extension refers to a tab pointing to a resource of that
extension, like chrome-extension://<id>/page.html.

Note: This does not address the case of terminated extensions, where we
don't currently close the extension tabs, or the case of embedded
iframes in non-extension pages.

BUG=531378
Committed: https://crrev.com/235441341064c329b155b09235d1aa8e5ea006f4
Cr-Commit-Position: refs/heads/master@{#421024}

[Devlin, 2016-09-21 00:54:45]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 00:55:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 02:57:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-21 02:57:23]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-09-21 17:48:28]

Description was changed from

==========
Unload
BUG=
==========

to

==========
[Extensions] Don't run unload listeners when an extension is removed

When an extension is removed, we close all the tabs for that extension*.
If any of these tabs had unload or beforeunload listeners, this close
process is asynchronous. However, if the listener performs any network
request, this would result in Chrome killing the tab for having an
invalid origin (the extension has been removed).

Instead, don't run these listeners at all. Since the extension has been
removed, there's no reason the code in the tab should run - the
extension should not be able to continue acting.

*a tab for an extension refers to a tab pointing to a resource of that 
extension, like chrome-extension://<id>/page.html.

Note: This does not address the case of terminated extensions, where we
don't currently close the extension tabs, or the case of embedded
iframes in non-extension pages.

BUG=531378
==========

[Devlin, 2016-09-21 17:48:36]

Patchset #1 (id:1) has been deleted

[Devlin, 2016-09-21 17:50:04]

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

[Devlin, 2016-09-21 17:50:05]

Hey Charlie, mind taking a look?

As mentioned in the CL description, this doesn't address terminated extensions
or embedded frames.  I'd like to handle terminated extensions separately since
it's more of a behavior change, and I'm not quite sure what to do in the case of
an embedded frame...

[Charlie Reis, 2016-09-21 18:29:33]

Thanks for the quick CL!  Mostly looks good with nits, with one question about
iframes.

I'm not concerned about terminated extensions, since they can't make network
requests.

I did confirm that an embedded extension iframe will be killed if it makes an
XHR after the extension is disabled/uninstalled.  Is there anything we can do
for that case?  I noticed that chrome://extensions does list all those extension
iframes, so we must track them somewhere.  I suppose just loading an error page
in them isn't sufficient, in case they put up a beforeunload dialog.  Removing
the frames would be nice, but even that is async and could lead to kills due to
races.  (I wonder if this is unavoidable?  Or if we can fail the XHR at a
different level before the kill?)

We probably don't need to fix this in this CL, but I'm curious if there's
anything quick we can do.

https://codereview.chromium.org/2355183002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_unload_browsertest.cc (right):

https://codereview.chromium.org/2355183002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_unload_browsertest.cc:32: ASSERT_EQ(1,
browser()->tab_strip_model()->count());
Sanity check: This failed before because the removal was async, right?

https://codereview.chromium.org/2355183002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_unload_browsertest.cc:38: // TODO(devlin):
We should replicate this behavior for terminated extensions.
I'm less concerned about terminated extensions (which can't make network
requests after the uninstall) than embedded iframes (which can).

https://codereview.chromium.org/2355183002/diff/20001/chrome/browser/ui/fast_...
File chrome/browser/ui/fast_unload_controller.cc (right):

https://codereview.chromium.org/2355183002/diff/20001/chrome/browser/ui/fast_...
chrome/browser/ui/fast_unload_controller.cc:86: GURL url =
contents->GetLastCommittedURL();
Let's add a comment, like: Don't run unload for extensions that have been
disabled or uninstalled, because they would be killed if they made network
requests.

https://codereview.chromium.org/2355183002/diff/20001/chrome/browser/ui/fast_...
chrome/browser/ui/fast_unload_controller.cc:88:
!extensions::ExtensionRegistry::Get(browser_->profile())
You would know better than I do, but does this need a if
defined(ENABLE_EXTENSIONS)?  I'm not sure which classes do and don't. 
(chrome/browser/browser.cc doesn't, but chrome/browser/site_details.cc does, for
example.)

https://codereview.chromium.org/2355183002/diff/20001/chrome/browser/ui/unloa...
File chrome/browser/ui/unload_controller.cc (right):

https://codereview.chromium.org/2355183002/diff/20001/chrome/browser/ui/unloa...
chrome/browser/ui/unload_controller.cc:56: GURL url =
contents->GetLastCommittedURL();
Same.

https://codereview.chromium.org/2355183002/diff/20001/chrome/test/data/extens...
File chrome/test/data/extensions/unload_listener/page.js (right):

https://codereview.chromium.org/2355183002/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/unload_listener/page.js:11: window.onbeforeunload =
onUnload;
I'd suggest leaving this line out, since beforeunload is different.  (I could
see testing both separately if we're concerned about it, but that may not be
necessary.)

[Devlin, 2016-09-22 00:35:21]

> I did confirm that an embedded extension iframe will be killed if it makes an
XHR after the extension is disabled/uninstalled.  Is there anything we can do
for that case?  I noticed that chrome://extensions does list all those extension
iframes, so we must track them somewhere.  I suppose just loading an error page
in them isn't sufficient, in case they put up a beforeunload dialog.  Removing
the frames would be nice, but even that is async and could lead to kills due to
races.  (I wonder if this is unavoidable?  Or if we can fail the XHR at a
different level before the kill?)

I don't know of anything synchronous we could do here, certainly. 
Asynchronously removing all the frames is an option.  I actually kind of like
the idea of just erroring all of them to 404s or something, but not sure how
easy that is to do while also avoiding running unload dialogs, etc.

https://codereview.chromium.org/2355183002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_unload_browsertest.cc (right):

https://codereview.chromium.org/2355183002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_unload_browsertest.cc:32: ASSERT_EQ(1,
browser()->tab_strip_model()->count());
On 2016/09/21 18:29:33, Charlie Reis (slow) wrote:
> Sanity check: This failed before because the removal was async, right?

Correct.

https://codereview.chromium.org/2355183002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_unload_browsertest.cc:38: // TODO(devlin):
We should replicate this behavior for terminated extensions.
On 2016/09/21 18:29:33, Charlie Reis (slow) wrote:
> I'm less concerned about terminated extensions (which can't make network
> requests after the uninstall) than embedded iframes (which can).

Don't quite follow.  It wouldn't be the extension making the request here, it
would be the tab.  So for instance:
Tab open to Extension A's options page
Extension A crashes (is terminated)
Tab remains open
Tab does some network request
Illegal origin (Extension A isn't enabled)

It's not immediately clear what the right course is - we deliberately don't
close all tabs when an extension crashes.  Maybe we could relax the origin check
for terminated extensions?  In either case, a discussion for another CL. :)

(Also, added iframes to the TODO)

https://codereview.chromium.org/2355183002/diff/20001/chrome/browser/ui/fast_...
File chrome/browser/ui/fast_unload_controller.cc (right):

https://codereview.chromium.org/2355183002/diff/20001/chrome/browser/ui/fast_...
chrome/browser/ui/fast_unload_controller.cc:86: GURL url =
contents->GetLastCommittedURL();
On 2016/09/21 18:29:33, Charlie Reis (slow) wrote:
> Let's add a comment, like: Don't run unload for extensions that have been
> disabled or uninstalled, because they would be killed if they made network
> requests.

Done.

https://codereview.chromium.org/2355183002/diff/20001/chrome/browser/ui/fast_...
chrome/browser/ui/fast_unload_controller.cc:88:
!extensions::ExtensionRegistry::Get(browser_->profile())
On 2016/09/21 18:29:33, Charlie Reis (slow) wrote:
> You would know better than I do, but does this need a if
> defined(ENABLE_EXTENSIONS)?  I'm not sure which classes do and don't. 
> (chrome/browser/browser.cc doesn't, but chrome/browser/site_details.cc does,
for
> example.)

Probably should, yes.  Added.  An OWNER here can double-check.

https://codereview.chromium.org/2355183002/diff/20001/chrome/browser/ui/unloa...
File chrome/browser/ui/unload_controller.cc (right):

https://codereview.chromium.org/2355183002/diff/20001/chrome/browser/ui/unloa...
chrome/browser/ui/unload_controller.cc:56: GURL url =
contents->GetLastCommittedURL();
On 2016/09/21 18:29:33, Charlie Reis (slow) wrote:
> Same.

Done.

https://codereview.chromium.org/2355183002/diff/20001/chrome/test/data/extens...
File chrome/test/data/extensions/unload_listener/page.js (right):

https://codereview.chromium.org/2355183002/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/unload_listener/page.js:11: window.onbeforeunload =
onUnload;
On 2016/09/21 18:29:33, Charlie Reis (slow) wrote:
> I'd suggest leaving this line out, since beforeunload is different.  (I could
> see testing both separately if we're concerned about it, but that may not be
> necessary.)

Is there any harm?  Neither one of these should change the outcome of removing
the tabs synchronously, which I think is what we want to ensure, right?

[Devlin, 2016-09-22 00:35:59]

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

[Devlin, 2016-09-22 00:36:00]

+sky for UI owners

[sky, 2016-09-22 13:11:37]

LGTM

[Charlie Reis, 2016-09-22 20:37:48]

On 2016/09/22 00:35:21, Devlin (OOO until sep 26) wrote:
> > I did confirm that an embedded extension iframe will be killed if it makes
an
> XHR after the extension is disabled/uninstalled.  Is there anything we can do
> for that case?  I noticed that chrome://extensions does list all those
extension
> iframes, so we must track them somewhere.  I suppose just loading an error
page
> in them isn't sufficient, in case they put up a beforeunload dialog.  Removing
> the frames would be nice, but even that is async and could lead to kills due
to
> races.  (I wonder if this is unavoidable?  Or if we can fail the XHR at a
> different level before the kill?)
> 
> I don't know of anything synchronous we could do here, certainly. 
> Asynchronously removing all the frames is an option.  I actually kind of like
> the idea of just erroring all of them to 404s or something, but not sure how
> easy that is to do while also avoiding running unload dialogs, etc.

Yeah, I don't know of any support for skipping {before}unload handlers.

Anyway, I don't think we'll solve the iframe case in this CL.  We'll have to
come back to it after we see what the RDH_ILLEGAL_ORIGIN kill numbers are like
after this CL lands.

LGTM, assuming you agree that terminated extensions aren't a problem (see
below).

https://codereview.chromium.org/2355183002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_unload_browsertest.cc (right):

https://codereview.chromium.org/2355183002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_unload_browsertest.cc:38: // TODO(devlin):
We should replicate this behavior for terminated extensions.
On 2016/09/22 00:35:21, Devlin (OOO until sep 26) wrote:
> On 2016/09/21 18:29:33, Charlie Reis (slow) wrote:
> > I'm less concerned about terminated extensions (which can't make network
> > requests after the uninstall) than embedded iframes (which can).
> 
> Don't quite follow.  It wouldn't be the extension making the request here, it
> would be the tab.  So for instance:
> Tab open to Extension A's options page
> Extension A crashes (is terminated)
> Tab remains open
> Tab does some network request
> Illegal origin (Extension A isn't enabled)
> 
> It's not immediately clear what the right course is - we deliberately don't
> close all tabs when an extension crashes.  Maybe we could relax the origin
check
> for terminated extensions?  In either case, a discussion for another CL. :)
> 
> (Also, added iframes to the TODO)

I'm confused.  In your example, the tab is showing extension A's options page,
which means that it becomes a sad tab when extension A crashes.  (There isn't a
separate renderer process for the tab.)  The tab remains open, but it can't make
network requests.  Am I missing something?

https://codereview.chromium.org/2355183002/diff/20001/chrome/test/data/extens...
File chrome/test/data/extensions/unload_listener/page.js (right):

https://codereview.chromium.org/2355183002/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/unload_listener/page.js:11: window.onbeforeunload =
onUnload;
On 2016/09/22 00:35:21, Devlin (OOO until sep 26) wrote:
> On 2016/09/21 18:29:33, Charlie Reis (slow) wrote:
> > I'd suggest leaving this line out, since beforeunload is different.  (I
could
> > see testing both separately if we're concerned about it, but that may not be
> > necessary.)
> 
> Is there any harm?  Neither one of these should change the outcome of removing
> the tabs synchronously, which I think is what we want to ensure, right?

I suppose it would matter more if it were possible for a beforeunload-only fix
to make the test pass, while unload is still broken.  But you're right the test
will only pass if neither beforeunload nor unload kick off an async close.  I'm
fine with leaving it.

[Devlin, 2016-09-26 17:54:17]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[Devlin, 2016-09-26 17:54:39]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 17:54:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 19:41:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-26 19:41:15]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Devlin, 2016-09-26 19:44:46]

https://codereview.chromium.org/2355183002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_unload_browsertest.cc (right):

https://codereview.chromium.org/2355183002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_unload_browsertest.cc:38: // TODO(devlin):
We should replicate this behavior for terminated extensions.
On 2016/09/22 20:37:47, Charlie Reis (slow) wrote:
> On 2016/09/22 00:35:21, Devlin (OOO until sep 26) wrote:
> > On 2016/09/21 18:29:33, Charlie Reis (slow) wrote:
> > > I'm less concerned about terminated extensions (which can't make network
> > > requests after the uninstall) than embedded iframes (which can).
> > 
> > Don't quite follow.  It wouldn't be the extension making the request here,
it
> > would be the tab.  So for instance:
> > Tab open to Extension A's options page
> > Extension A crashes (is terminated)
> > Tab remains open
> > Tab does some network request
> > Illegal origin (Extension A isn't enabled)
> > 
> > It's not immediately clear what the right course is - we deliberately don't
> > close all tabs when an extension crashes.  Maybe we could relax the origin
> check
> > for terminated extensions?  In either case, a discussion for another CL. :)
> > 
> > (Also, added iframes to the TODO)
> 
> I'm confused.  In your example, the tab is showing extension A's options page,
> which means that it becomes a sad tab when extension A crashes.  (There isn't
a
> separate renderer process for the tab.)  The tab remains open, but it can't
make
> network requests.  Am I missing something?

Ah, right.  I was distracted by the browser code to deliberately *not* close
tabs for crashed extensions, and assumed it was so that reloading the extension
was a reasonable option.  But you're right that these would all be sad tabs at
this point.  Updated to not mention terminated extensions.

[Devlin, 2016-09-26 19:44:52]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2016-09-26 19:44:52]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2355183002/#ps60001
(title: "Remove terminated extensions reference")

[commit-bot: I haz the power, 2016-09-26 19:45:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 22:39:28]

Description was changed from

==========
[Extensions] Don't run unload listeners when an extension is removed

When an extension is removed, we close all the tabs for that extension*.
If any of these tabs had unload or beforeunload listeners, this close
process is asynchronous. However, if the listener performs any network
request, this would result in Chrome killing the tab for having an
invalid origin (the extension has been removed).

Instead, don't run these listeners at all. Since the extension has been
removed, there's no reason the code in the tab should run - the
extension should not be able to continue acting.

*a tab for an extension refers to a tab pointing to a resource of that 
extension, like chrome-extension://<id>/page.html.

Note: This does not address the case of terminated extensions, where we
don't currently close the extension tabs, or the case of embedded
iframes in non-extension pages.

BUG=531378
==========

to

==========
[Extensions] Don't run unload listeners when an extension is removed

When an extension is removed, we close all the tabs for that extension*.
If any of these tabs had unload or beforeunload listeners, this close
process is asynchronous. However, if the listener performs any network
request, this would result in Chrome killing the tab for having an
invalid origin (the extension has been removed).

Instead, don't run these listeners at all. Since the extension has been
removed, there's no reason the code in the tab should run - the
extension should not be able to continue acting.

*a tab for an extension refers to a tab pointing to a resource of that 
extension, like chrome-extension://<id>/page.html.

Note: This does not address the case of terminated extensions, where we
don't currently close the extension tabs, or the case of embedded
iframes in non-extension pages.

BUG=531378
==========

[commit-bot: I haz the power, 2016-09-26 22:39:29]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-09-26 22:43:44]

Description was changed from

==========
[Extensions] Don't run unload listeners when an extension is removed

When an extension is removed, we close all the tabs for that extension*.
If any of these tabs had unload or beforeunload listeners, this close
process is asynchronous. However, if the listener performs any network
request, this would result in Chrome killing the tab for having an
invalid origin (the extension has been removed).

Instead, don't run these listeners at all. Since the extension has been
removed, there's no reason the code in the tab should run - the
extension should not be able to continue acting.

*a tab for an extension refers to a tab pointing to a resource of that 
extension, like chrome-extension://<id>/page.html.

Note: This does not address the case of terminated extensions, where we
don't currently close the extension tabs, or the case of embedded
iframes in non-extension pages.

BUG=531378
==========

to

==========
[Extensions] Don't run unload listeners when an extension is removed

When an extension is removed, we close all the tabs for that extension*.
If any of these tabs had unload or beforeunload listeners, this close
process is asynchronous. However, if the listener performs any network
request, this would result in Chrome killing the tab for having an
invalid origin (the extension has been removed).

Instead, don't run these listeners at all. Since the extension has been
removed, there's no reason the code in the tab should run - the
extension should not be able to continue acting.

*a tab for an extension refers to a tab pointing to a resource of that
extension, like chrome-extension://<id>/page.html.

Note: This does not address the case of terminated extensions, where we
don't currently close the extension tabs, or the case of embedded
iframes in non-extension pages.

BUG=531378

Committed: https://crrev.com/235441341064c329b155b09235d1aa8e5ea006f4
Cr-Commit-Position: refs/heads/master@{#421024}
==========

[commit-bot: I haz the power, 2016-09-26 22:43:45]

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