Fix a crash in PageActionImageView when extensions are reloaded due to incognito settings changing.

Fix a crash in PageActionImageView when extensions are reloaded due to incognito settings changing.

BUG=99736
TEST=Precise reproduction steps can be found in the bug description (first comment).
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109011

[Aaron Boodman, 2011-10-20 15:21:49]

http://codereview.chromium.org/8333021/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/8333021/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:2031: ExtensionList::iterator
iter = std::find(extensions_.begin(),
Can we just call ReloadExtension() instead? That is what SetFileAccessAllowed()
(below) does, which is similar.

[Finnur, 2011-10-20 15:29:15]

I guess that's a question for Matt, no (I think he wrote it)?

[Finnur, 2011-10-21 15:25:39]

On 2011/10/20 15:29:15, Finnur wrote:
> I guess that's a question for Matt, no (I think he wrote it)?

Matt, what's your take on that?

[Matt Perry, 2011-10-24 18:48:56]

http://codereview.chromium.org/8333021/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/8333021/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:2031: ExtensionList::iterator
iter = std::find(extensions_.begin(),
On 2011/10/20 15:21:49, Aaron Boodman wrote:
> Can we just call ReloadExtension() instead? That is what
SetFileAccessAllowed()
> (below) does, which is similar.

Honestly, I don't know. I didn't realize the two used different tactics. Only
option is to try it and see what breaks. There could be some system that depends
on it being done this way.

[Aaron Boodman, 2011-10-24 18:50:42]

On Mon, Oct 24, 2011 at 11:48 AM,  <mpcomplete@chromium.org> wrote:
>
>
http://codereview.chromium.org/8333021/diff/4001/chrome/browser/extensions/ex...
> File chrome/browser/extensions/extension_service.cc (right):
>
>
http://codereview.chromium.org/8333021/diff/4001/chrome/browser/extensions/ex...
> chrome/browser/extensions/extension_service.cc:2031:
> ExtensionList::iterator iter = std::find(extensions_.begin(),
> On 2011/10/20 15:21:49, Aaron Boodman wrote:
>>
>> Can we just call ReloadExtension() instead? That is what
>
> SetFileAccessAllowed()
>>
>> (below) does, which is similar.
>
> Honestly, I don't know. I didn't realize the two used different tactics.
> Only option is to try it and see what breaks. There could be some system
> that depends on it being done this way.

Please just try ReloadExtension().

- a

[Finnur, 2011-10-24 20:28:42]

Will do tomorrow.

On Mon, Oct 24, 2011 at 18:50, Aaron Boodman <aa@chromium.org> wrote:

> On Mon, Oct 24, 2011 at 11:48 AM,  <mpcomplete@chromium.org> wrote:
> >
> >
>
http://codereview.chromium.org/8333021/diff/4001/chrome/browser/extensions/ex...
> > File chrome/browser/extensions/extension_service.cc (right):
> >
> >
>
http://codereview.chromium.org/8333021/diff/4001/chrome/browser/extensions/ex...
> > chrome/browser/extensions/extension_service.cc:2031:
> > ExtensionList::iterator iter = std::find(extensions_.begin(),
> > On 2011/10/20 15:21:49, Aaron Boodman wrote:
> >>
> >> Can we just call ReloadExtension() instead? That is what
> >
> > SetFileAccessAllowed()
> >>
> >> (below) does, which is similar.
> >
> > Honestly, I don't know. I didn't realize the two used different tactics.
> > Only option is to try it and see what breaks. There could be some system
> > that depends on it being done this way.
>
> Please just try ReloadExtension().
>
> - a
>

[Finnur, 2011-10-26 09:47:09]

Hmmm... ReloadExtension uses ExtensionProcessManager, which for a testing
profile is NULL so tests like ExtensionServiceTest.ProcessSyncDataSettings start
crashing if I switch to ReloadExtension...

[Finnur, 2011-10-26 23:58:43]

I'll look at this a bit more tomorrow, but in the meantime -- any bright ideas
for how I should approach this?

[Matt Perry, 2011-10-27 00:14:36]

On 2011/10/26 23:58:43, Finnur wrote:
> I'll look at this a bit more tomorrow, but in the meantime -- any bright ideas
> for how I should approach this?

I guess no unit tests exercise the SetAllowFileAccess code path?

One option is to add a CreateExtensionProcessManager to TestingProfile (like
other Create* methods in that class), and call it for the tests that need it.

[Finnur, 2011-10-27 15:23:31]

akalin/bolms, see question below.

> I guess no unit tests exercise the SetAllowFileAccess 
> code path?

Correct.

> One option is to add a CreateExtensionProcessManager 
> to TestingProfile (like other Create* methods in that
> class), and call it for the tests that need it.

Thanks for the suggestion. That's along the lines of what I had in mind to try
out. Please see updated CL, which now passes test.

For posterity, the tests that failed during the making of this CL were:
ExtensionServiceTest.ProcessSyncDataSettings (unit test)
TranslateManagerTest.BeforeTranslateExtraButtons (browser test)

http://codereview.chromium.org/8333021/diff/23009/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (left):

http://codereview.chromium.org/8333021/diff/23009/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1951: SetIsIncognitoEnabled(id,
extension_sync_data.incognito_enabled());

I had to move this down to the bottom of the function because using the
extension pointer is not safe after calling SetIsIncognitoEnabled (it reloads
and invalidates the extension pointer).

Can either bolms or akalin verify it is OK to move this code?

http://codereview.chromium.org/8333021/diff/23009/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:2022:
SyncExtensionChangeIfNeeded(*extension);
These last two lines are not needed because ReloadExtension does this.

[Aaron Boodman, 2011-10-27 16:57:35]

From my point of view, this LG.

Thanks for being patient and finding a good solution.

http://codereview.chromium.org/8333021/diff/23009/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/page_action_image_view.cc (right):

http://codereview.chromium.org/8333021/diff/23009/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/page_action_image_view.cc:250: DCHECK(type
== chrome::NOTIFICATION_EXTENSION_UNLOADED);
Can use _EQ.

[Matt Perry, 2011-10-27 18:05:28]

lgtm

http://codereview.chromium.org/8333021/diff/23009/chrome/test/base/testing_pr...
File chrome/test/base/testing_profile.cc (right):

http://codereview.chromium.org/8333021/diff/23009/chrome/test/base/testing_pr...
chrome/test/base/testing_profile.cc:347:
extension_process_manager_.reset(ExtensionProcessManager::Create(this));
we might not want to do this all the time. i think it will spawn extension
processes every time an extension is loaded. consider moving it to a separate
Create helper and only call it from the tests that need it.

[Ben Olmstead, 2011-10-27 19:59:34]

On 2011/10/27 15:23:31, Finnur wrote:
> akalin/bolms, see question below.
> 
> > I guess no unit tests exercise the SetAllowFileAccess 
> > code path?
> 
> Correct.
> 
> > One option is to add a CreateExtensionProcessManager 
> > to TestingProfile (like other Create* methods in that
> > class), and call it for the tests that need it.
> 
> Thanks for the suggestion. That's along the lines of what I had in mind to try
> out. Please see updated CL, which now passes test.
> 
> For posterity, the tests that failed during the making of this CL were:
> ExtensionServiceTest.ProcessSyncDataSettings (unit test)
> TranslateManagerTest.BeforeTranslateExtraButtons (browser test)
> 
>
http://codereview.chromium.org/8333021/diff/23009/chrome/browser/extensions/e...
> File chrome/browser/extensions/extension_service.cc (left):
> 
>
http://codereview.chromium.org/8333021/diff/23009/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_service.cc:1951: SetIsIncognitoEnabled(id,
> extension_sync_data.incognito_enabled());
> 
> I had to move this down to the bottom of the function because using the
> extension pointer is not safe after calling SetIsIncognitoEnabled (it reloads
> and invalidates the extension pointer).
> 
> Can either bolms or akalin verify it is OK to move this code?
> 
>
http://codereview.chromium.org/8333021/diff/23009/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_service.cc:2022:
> SyncExtensionChangeIfNeeded(*extension);
> These last two lines are not needed because ReloadExtension does this.

The new version does not update sync if the extension is disabled when
SetIsIncognitoEnabled is called.  I don't know what can trigger that, aside from
sync.  (I also don't know if there will be new ways in the future.)

It's also a bit of a bug that ReloadExtension updates sync at all--it really
shouldn't, given that it doesn't change the logical status of the extension.

[Finnur, 2011-10-27 20:31:35]

What do you recommend, Ben? Re-adding these two lines?

Add the On 2011/10/27 19:59:34, Ben Olmstead wrote:
> On 2011/10/27 15:23:31, Finnur wrote:
> > akalin/bolms, see question below.
> > 
> > > I guess no unit tests exercise the SetAllowFileAccess 
> > > code path?
> > 
> > Correct.
> > 
> > > One option is to add a CreateExtensionProcessManager 
> > > to TestingProfile (like other Create* methods in that
> > > class), and call it for the tests that need it.
> > 
> > Thanks for the suggestion. That's along the lines of what I had in mind to
try
> > out. Please see updated CL, which now passes test.
> > 
> > For posterity, the tests that failed during the making of this CL were:
> > ExtensionServiceTest.ProcessSyncDataSettings (unit test)
> > TranslateManagerTest.BeforeTranslateExtraButtons (browser test)
> > 
> >
>
http://codereview.chromium.org/8333021/diff/23009/chrome/browser/extensions/e...
> > File chrome/browser/extensions/extension_service.cc (left):
> > 
> >
>
http://codereview.chromium.org/8333021/diff/23009/chrome/browser/extensions/e...
> > chrome/browser/extensions/extension_service.cc:1951:
SetIsIncognitoEnabled(id,
> > extension_sync_data.incognito_enabled());
> > 
> > I had to move this down to the bottom of the function because using the
> > extension pointer is not safe after calling SetIsIncognitoEnabled (it
reloads
> > and invalidates the extension pointer).
> > 
> > Can either bolms or akalin verify it is OK to move this code?
> > 
> >
>
http://codereview.chromium.org/8333021/diff/23009/chrome/browser/extensions/e...
> > chrome/browser/extensions/extension_service.cc:2022:
> > SyncExtensionChangeIfNeeded(*extension);
> > These last two lines are not needed because ReloadExtension does this.
> 
> The new version does not update sync if the extension is disabled when
> SetIsIncognitoEnabled is called.  I don't know what can trigger that, aside
from
> sync.  (I also don't know if there will be new ways in the future.)
> 
> It's also a bit of a bug that ReloadExtension updates sync at all--it really
> shouldn't, given that it doesn't change the logical status of the extension.

[Ben Olmstead, 2011-10-27 20:34:02]

On 2011/10/27 20:31:35, Finnur wrote:
> What do you recommend, Ben? Re-adding these two lines?

That seems safest.  I think the performance impact of doing an extra sync update
should be negligible, though I haven't profiled it.

> Add the On 2011/10/27 19:59:34, Ben Olmstead wrote:
> > On 2011/10/27 15:23:31, Finnur wrote:
> > > akalin/bolms, see question below.
> > > 
> > > > I guess no unit tests exercise the SetAllowFileAccess 
> > > > code path?
> > > 
> > > Correct.
> > > 
> > > > One option is to add a CreateExtensionProcessManager 
> > > > to TestingProfile (like other Create* methods in that
> > > > class), and call it for the tests that need it.
> > > 
> > > Thanks for the suggestion. That's along the lines of what I had in mind to
> try
> > > out. Please see updated CL, which now passes test.
> > > 
> > > For posterity, the tests that failed during the making of this CL were:
> > > ExtensionServiceTest.ProcessSyncDataSettings (unit test)
> > > TranslateManagerTest.BeforeTranslateExtraButtons (browser test)
> > > 
> > >
> >
>
http://codereview.chromium.org/8333021/diff/23009/chrome/browser/extensions/e...
> > > File chrome/browser/extensions/extension_service.cc (left):
> > > 
> > >
> >
>
http://codereview.chromium.org/8333021/diff/23009/chrome/browser/extensions/e...
> > > chrome/browser/extensions/extension_service.cc:1951:
> SetIsIncognitoEnabled(id,
> > > extension_sync_data.incognito_enabled());
> > > 
> > > I had to move this down to the bottom of the function because using the
> > > extension pointer is not safe after calling SetIsIncognitoEnabled (it
> reloads
> > > and invalidates the extension pointer).
> > > 
> > > Can either bolms or akalin verify it is OK to move this code?
> > > 
> > >
> >
>
http://codereview.chromium.org/8333021/diff/23009/chrome/browser/extensions/e...
> > > chrome/browser/extensions/extension_service.cc:2022:
> > > SyncExtensionChangeIfNeeded(*extension);
> > > These last two lines are not needed because ReloadExtension does this.
> > 
> > The new version does not update sync if the extension is disabled when
> > SetIsIncognitoEnabled is called.  I don't know what can trigger that, aside
> from
> > sync.  (I also don't know if there will be new ways in the future.)
> > 
> > It's also a bit of a bug that ReloadExtension updates sync at all--it really
> > shouldn't, given that it doesn't change the logical status of the extension.

[akalin, 2011-10-31 20:26:48]

http://codereview.chromium.org/8333021/diff/23009/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (left):

http://codereview.chromium.org/8333021/diff/23009/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1951: SetIsIncognitoEnabled(id,
extension_sync_data.incognito_enabled());
On 2011/10/27 15:23:32, Finnur wrote:
> 
> I had to move this down to the bottom of the function because using the
> extension pointer is not safe after calling SetIsIncognitoEnabled (it reloads
> and invalidates the extension pointer).
> 
> Can either bolms or akalin verify it is OK to move this code?

I'd prefer leaving this block as is, but caching any info you need before
calling SetIsIncognitoEnabled (and adding a comment why).

It looks like it's only needed to see if the extension exists, and if so, to
compare its version.

[Finnur, 2011-11-02 16:26:32]

Updated the CL.

Aaron: I changed the DCHECK to DCHECK_EQ.
Ben: Readded the sync code.
akalin: Cached the extension info before reloading.
Matt: Made the creation require explicit function call. In order to call the new
function I also had to change the Profile to a TestingProfile pointer, which
requires casting in a couple of unrelated places.

[Matt Perry, 2011-11-02 19:01:31]

lgtm

http://codereview.chromium.org/8333021/diff/34013/chrome/browser/extensions/n...
File chrome/browser/extensions/network_delay_listener_unittest.cc (right):

http://codereview.chromium.org/8333021/diff/34013/chrome/browser/extensions/n...
chrome/browser/extensions/network_delay_listener_unittest.cc:139:
content::Source<Profile>(reinterpret_cast<Profile*>(profile_.get())),
is the cast really necessary? TestingProfile derives from Profile so usually the
cast is implicit.

if you do need a cast, use static_cast, since it's a cast within a class
hierarchy.

[Ben Olmstead, 2011-11-02 19:44:23]

Restoring the sync bits lgtm.

[Finnur, 2011-11-02 21:14:43]

static_cast was the first thing I tried, but it didn't work -- I don't remember
the error off the top of my head, but I'll try again tomorrow. Maybe I made some
mistake.

akalin, is Ben's LG enough for you?

On 2011/11/02 19:01:31, Matt Perry wrote:
> lgtm
> 
>
http://codereview.chromium.org/8333021/diff/34013/chrome/browser/extensions/n...
> File chrome/browser/extensions/network_delay_listener_unittest.cc (right):
> 
>
http://codereview.chromium.org/8333021/diff/34013/chrome/browser/extensions/n...
> chrome/browser/extensions/network_delay_listener_unittest.cc:139:
> content::Source<Profile>(reinterpret_cast<Profile*>(profile_.get())),
> is the cast really necessary? TestingProfile derives from Profile so usually
the
> cast is implicit.
> 
> if you do need a cast, use static_cast, since it's a cast within a class
> hierarchy.

[Finnur, 2011-11-03 10:49:23]

akalin, is Ben's LG good enough for you?

http://codereview.chromium.org/8333021/diff/34013/chrome/browser/extensions/n...
File chrome/browser/extensions/network_delay_listener_unittest.cc (right):

http://codereview.chromium.org/8333021/diff/34013/chrome/browser/extensions/n...
chrome/browser/extensions/network_delay_listener_unittest.cc:139:
content::Source<Profile>(reinterpret_cast<Profile*>(profile_.get())),
Implicit casting didn't work (complained the types were unrelated) but after
digging into it some more it turned out to be due to forward decls of the
Profile class. Fixed.

On 2011/11/02 19:01:31, Matt Perry wrote:
> is the cast really necessary? TestingProfile derives from Profile so usually
the
> cast is implicit.
> 
> if you do need a cast, use static_cast, since it's a cast within a class
> hierarchy.