[Extensions] Log instances of invalid extensions being added

[Extensions] Log instances of invalid extensions being added

In theory, we should never add an extension with an invalid location
(install source). However, in practice, bugs imply that this might be
able to happen.

Add some more tracking in ExtensionService::AddExtension to both upload
a crash dump and gracefully early-out any case where we try to add
invalid extensions.

BUG=692069
BUG=698736
Review-Url: https://codereview.chromium.org/2738553002
Cr-Commit-Position: refs/heads/master@{#455897}
Committed: https://chromium.googlesource.com/chromium/src/+/08ada0f645a7440d60b0004392b6330582c1da7e

[Devlin, 2017-03-06 22:09:59]

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

[Devlin, 2017-03-06 22:10:00]

https://codereview.chromium.org/2738553002/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/2738553002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1492:
base::debug::DumpWithoutCrashing();
I don't think I've ever actually used this before - do I need to do anything
else here?

[lazyboy, 2017-03-06 23:08:11]

/cc lfg about base::debug::Alias()

https://codereview.chromium.org/2738553002/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/2738553002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1487: // TODO(devlin): We should
*never* add an extension with an invalid
nit: the TODO should also be explicitly talking about removing this temporary
call to DumpWithoutCrashing.

https://codereview.chromium.org/2738553002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1492:
base::debug::DumpWithoutCrashing();
On 2017/03/06 22:10:00, Devlin wrote:
> I don't think I've ever actually used this before - do I need to do anything
> else here?

I think this is fine, however, the Alias on the line above needs tweaking, as
far as I understand. You *probably* want stuff on the stack (on a local
variable) to be sure to record. Also |extension| contains too much info on it to
be recorded properly (I assume there's a limit?), you want to specifically pull
out parts/members of extension variable that would help you investigate the
crash, e.g. put extension->id() in a char[] and Alias that.

Also: return; here?

[Devlin, 2017-03-07 01:29:32]

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

[commit-bot: I haz the power, 2017-03-07 01:30:43]

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

[Devlin, 2017-03-07 01:31:40]

https://codereview.chromium.org/2738553002/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/2738553002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1487: // TODO(devlin): We should
*never* add an extension with an invalid
On 2017/03/06 23:08:11, lazyboy wrote:
> nit: the TODO should also be explicitly talking about removing this temporary
> call to DumpWithoutCrashing.

Done.

https://codereview.chromium.org/2738553002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1492:
base::debug::DumpWithoutCrashing();
On 2017/03/06 23:08:11, lazyboy wrote:
> On 2017/03/06 22:10:00, Devlin wrote:
> > I don't think I've ever actually used this before - do I need to do anything
> > else here?
> 
> I think this is fine, however, the Alias on the line above needs tweaking, as
> far as I understand. You *probably* want stuff on the stack (on a local
> variable) to be sure to record. Also |extension| contains too much info on it
to
> be recorded properly (I assume there's a limit?), you want to specifically
pull
> out parts/members of extension variable that would help you investigate the
> crash, e.g. put extension->id() in a char[] and Alias that.

Ah, got it.  Done (I think) in the latest patch set.

> Also: return; here?

Right, done.

[lfg, 2017-03-07 17:00:29]

lfg@chromium.org changed reviewers:
+ lfg@chromium.org

[lfg, 2017-03-07 17:00:30]

https://codereview.chromium.org/2738553002/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/2738553002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1492:
base::debug::DumpWithoutCrashing();
On 2017/03/07 01:31:40, Devlin wrote:
> On 2017/03/06 23:08:11, lazyboy wrote:
> > On 2017/03/06 22:10:00, Devlin wrote:
> > > I don't think I've ever actually used this before - do I need to do
anything
> > > else here?
> > 
> > I think this is fine, however, the Alias on the line above needs tweaking,
as
> > far as I understand. You *probably* want stuff on the stack (on a local
> > variable) to be sure to record. Also |extension| contains too much info on
it
> to
> > be recorded properly (I assume there's a limit?), you want to specifically
> pull
> > out parts/members of extension variable that would help you investigate the
> > crash, e.g. put extension->id() in a char[] and Alias that.
> 
> Ah, got it.  Done (I think) in the latest patch set.
> 
> > Also: return; here?
> 
> Right, done.

Yes, this is fine. In most cases now, you don't even need to alias anymore
because the dump tool captures indirect data referenced from the stack (see bug
587139), but in this case, you do need the alias to capture the
Manifest::location, which is a pointer owned by the Extension and won't be
referenced on the stack.

[lazyboy, 2017-03-08 19:01:01]

https://codereview.chromium.org/2738553002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/2738553002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1492: char extension_id_copy[32];
I think you need 33 here, b/c of strlcpy.

[Devlin, 2017-03-09 01:23:38]

https://codereview.chromium.org/2738553002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/2738553002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1492: char extension_id_copy[32];
On 2017/03/08 19:01:01, lazyboy wrote:
> I think you need 33 here, b/c of strlcpy.

done.

[Devlin, 2017-03-09 01:24:15]

rdevlin.cronin@chromium.org changed reviewers:
+ atwilson@chromium.org, battre@chromium.org

[Devlin, 2017-03-09 01:24:15]

+battre for profile_resetter
+atwilson for background

Just updating tests to not use invalid manifest locations.

[Andrew T Wilson (Slow), 2017-03-09 09:08:36]

background/ lgtm

[lazyboy, 2017-03-09 18:16:24]

Sorry this slipped through, lgtm

[battre, 2017-03-09 20:53:41]

lgtm

[Devlin, 2017-03-09 21:13:46]

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

[commit-bot: I haz the power, 2017-03-09 21:14:11]

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, 2017-03-09 23:29:56]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489094026489650,
"parent_rev": "4fe3a084bcf7a2782c29498761186eac282306b5", "commit_rev":
"08ada0f645a7440d60b0004392b6330582c1da7e"}

[commit-bot: I haz the power, 2017-03-09 23:30:38]

Description was changed from

==========
[Extensions] Log instances of invalid extensions being added

In theory, we should never add an extension with an invalid location
(install source). However, in practice, bugs imply that this might be
able to happen.

Add some more tracking in ExtensionService::AddExtension to both upload
a crash dump and gracefully early-out any case where we try to add
invalid extensions.

BUG=692069
BUG=698736
==========

to

==========
[Extensions] Log instances of invalid extensions being added

In theory, we should never add an extension with an invalid location
(install source). However, in practice, bugs imply that this might be
able to happen.

Add some more tracking in ExtensionService::AddExtension to both upload
a crash dump and gracefully early-out any case where we try to add
invalid extensions.

BUG=692069
BUG=698736

Review-Url: https://codereview.chromium.org/2738553002
Cr-Commit-Position: refs/heads/master@{#455897}
Committed:
https://chromium.googlesource.com/chromium/src/+/08ada0f645a7440d60b0004392b6...
==========

[commit-bot: I haz the power, 2017-03-09 23:30:39]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/08ada0f645a7440d60b0004392b6...