[Extensions] Observe unloading extensions in the TabHelper

[Extensions] Observe unloading extensions in the TabHelper

The extension TabHelper can have an associated extension if the
WebContents holds a hosted app. Watch for that extension being
unloaded and reset the reference to the extension if it is so that
we don't UAF it.

BUG=616057
BUG=616113
Committed: https://crrev.com/1f047f7b10be249ef22ecd45faf053592b4bf0ea
Cr-Commit-Position: refs/heads/master@{#398072}

[Devlin, 2016-06-03 00:06:33]

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

[lazyboy, 2016-06-03 01:43:56]

lgtm

https://codereview.chromium.org/2037873002/diff/1/chrome/browser/extensions/t...
File chrome/browser/extensions/tab_helper.cc (right):

https://codereview.chromium.org/2037873002/diff/1/chrome/browser/extensions/t...
chrome/browser/extensions/tab_helper.cc:173: if (extension_app_) {
lgtm given this is not true:
Is it possible that two extensions get associated with the same TabHelper?
SetExtensionApp(extension1)
SetExtensionApp(extension2)
Uninstall -> extension 2
Uninstall -> extension 1, the DCHECK in line 545 below will fail.

[Devlin, 2016-06-06 16:50:22]

https://codereview.chromium.org/2037873002/diff/1/chrome/browser/extensions/t...
File chrome/browser/extensions/tab_helper.cc (right):

https://codereview.chromium.org/2037873002/diff/1/chrome/browser/extensions/t...
chrome/browser/extensions/tab_helper.cc:173: if (extension_app_) {
On 2016/06/03 01:43:56, lazyboy wrote:
> lgtm given this is not true:
> Is it possible that two extensions get associated with the same TabHelper?
> SetExtensionApp(extension1)
> SetExtensionApp(extension2)
> Uninstall -> extension 2
> Uninstall -> extension 1, the DCHECK in line 545 below will fail.

That flow might be able to happen (if e.g. you navigate from one hosted app to
another), but that shouldn't trigger the DCHECK:
SetExtensionApp(extension1) -> register observer, extension_app_ = extension1
SetExtensionApp(extension2) -> extension_app_ = extension2
Uninstall(extension2) -> extension_app_ = nullptr, unregister observer
Uninstall(extension1) -> since unregistered, never see notification

However, a similar flow would cause a DCHECK:
SetExtensionApp(extension1)
SetExtensionApp(nullptr)
Uninstall(extension1)

I've add logic to SetExtensionApp() to remove the observer if setting to nullptr
to address it.

[lazyboy, 2016-06-06 18:10:49]

lgtm

https://codereview.chromium.org/2037873002/diff/1/chrome/browser/extensions/t...
File chrome/browser/extensions/tab_helper.cc (right):

https://codereview.chromium.org/2037873002/diff/1/chrome/browser/extensions/t...
chrome/browser/extensions/tab_helper.cc:173: if (extension_app_) {
On 2016/06/06 16:50:22, Devlin wrote:
> On 2016/06/03 01:43:56, lazyboy wrote:
> > lgtm given this is not true:
> > Is it possible that two extensions get associated with the same TabHelper?
> > SetExtensionApp(extension1)
> > SetExtensionApp(extension2)
> > Uninstall -> extension 2
> > Uninstall -> extension 1, the DCHECK in line 545 below will fail.
> 
> That flow might be able to happen (if e.g. you navigate from one hosted app to
> another), but that shouldn't trigger the DCHECK:
> SetExtensionApp(extension1) -> register observer, extension_app_ = extension1
> SetExtensionApp(extension2) -> extension_app_ = extension2
> Uninstall(extension2) -> extension_app_ = nullptr, unregister observer
> Uninstall(extension1) -> since unregistered, never see notification
> 
> However, a similar flow would cause a DCHECK:
> SetExtensionApp(extension1)
> SetExtensionApp(nullptr)
> Uninstall(extension1)
> 
> I've add logic to SetExtensionApp() to remove the observer if setting to
nullptr
> to address it.

Acknowledged.

[Devlin, 2016-06-06 18:22:48]

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

[commit-bot: I haz the power, 2016-06-06 18:23:49]

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

[commit-bot: I haz the power, 2016-06-06 18:30:32]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-06 18:33:07]

Description was changed from

==========
[Extensions] Observe unloading extensions in the TabHelper

The extension TabHelper can have an associated extension if the
WebContents holds a hosted app. Watch for that extension being
unloaded and reset the reference to the extension if it is so that
we don't UAF it.

BUG=616057
BUG=616113
==========

to

==========
[Extensions] Observe unloading extensions in the TabHelper

The extension TabHelper can have an associated extension if the
WebContents holds a hosted app. Watch for that extension being
unloaded and reset the reference to the extension if it is so that
we don't UAF it.

BUG=616057
BUG=616113

Committed: https://crrev.com/1f047f7b10be249ef22ecd45faf053592b4bf0ea
Cr-Commit-Position: refs/heads/master@{#398072}
==========

[commit-bot: I haz the power, 2016-06-06 18:33:08]

Patchset 2 (id:??) landed as
https://crrev.com/1f047f7b10be249ef22ecd45faf053592b4bf0ea
Cr-Commit-Position: refs/heads/master@{#398072}