Fix crash in ExtensionService::SetIsIncognitoEnabled.

Fix crash in ExtensionService::SetIsIncognitoEnabled.

Make a copy of the extension id from within ExtensionService::SetIsIncognitoEnabled, since the const ref may get wiped out when reloading the extension.

BUG=103762
TEST=no crash when toggling incognito mode for an extension

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109676

[Finnur, 2011-11-10 21:48:44]

http://codereview.chromium.org/8529001/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/8529001/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.cc:1459: std::string extension_id,
bool enabled) {
I wonder if people will be tempted to change this back to const ref, since it
isn't obvious that a copy is needed and therefore people would look at this as a
potential speedup opportunity? 

Should we document that this must be a copy because callers will likely not make
a copy of the value before calling us?

Or should we take a const ref, but make a copy ourselves (line 1478) before we
reload?

Or should we have the only caller (in this file at least -- are there others?)
make a copy of the ID as well, since we already cache some extension data (line
1409).

Just a thought.

http://codereview.chromium.org/8529001/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/8529001/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.h:194: virtual void
SetIsIncognitoEnabled(std::string extension_id, bool enabled);
This is virtual, don't you need to change signatures elsewhere too? But before
you do, see comment in .cc file.

[Finnur, 2011-11-11 09:54:08]

jstritar: Can you change the BUG reference in the CL description to 103762, as
it more accurately describes the bug?

[jstritar, 2011-11-11 15:53:15]

Good advice, very misleading. I made a copy from within SetIsIncognitoEnabled
since the issue is with clients of ReloadExtension.

[Finnur, 2011-11-11 16:01:07]

LGTM, one nit (feel free to ignore)

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

http://codereview.chromium.org/8529001/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:1463: std::string id =
extension_id;
nit: It would have been enough to make the copy right before calling reload()
(then you are not needlessly copying for the cases where we return early, like
lines 1469, 1477).

No biggie though.

[jstritar, 2011-11-11 16:31:37]

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

http://codereview.chromium.org/8529001/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:1463: std::string id =
extension_id;
On 2011/11/11 16:01:07, Finnur wrote:
> nit: It would have been enough to make the copy right before calling reload()
> (then you are not needlessly copying for the cases where we return early, like
> lines 1469, 1477).
> 
> No biggie though.

Done.

[commit-bot: I haz the power, 2011-11-11 16:31:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jstritar@chromium.org/8529001/8001

[commit-bot: I haz the power, 2011-11-11 18:04:20]

Try job failure for 8529001-8001 (retry) on win_rel for step "ui_tests" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-11-11 18:19:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jstritar@chromium.org/8529001/8001

[commit-bot: I haz the power, 2011-11-11 19:41:35]

Change committed as 109676