[Extensions] Log instances of invalid extensions being added

chrome/browser/extensions/extension_service.cc

Index: chrome/browser/extensions/extension_service.cc
diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index 7a283b58138b0caa20f76fe9d913e34e5135b120..fa74426259006adfe1ec7c49e0566b9452d3ce38 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -13,6 +13,8 @@
 #include <utility>
 
 #include "base/command_line.h"
+#include "base/debug/alias.h"
+#include "base/debug/dump_without_crashing.h"
 #include "base/location.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/single_thread_task_runner.h"
@@ -1481,6 +1483,15 @@ void ExtensionService::OnLoadedInstalledExtensions() {
 }
 
 void ExtensionService::AddExtension(const Extension* extension) {
+  if (!Manifest::IsValidLocation(extension->location())) {
+    // TODO(devlin): We should *never* add an extension with an invalid

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

nit: the TODO should also be explicitly talking about removing this temporary
call to DumpWithoutCrashing.

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

Done.

+    // location, but some bugs (e.g. crbug.com/692069) seem to indicate we do.
+    // Track down the cases when this can happen.
+    NOTREACHED();
+    base::debug::Alias(extension);
+    base::debug::DumpWithoutCrashing();

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

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]

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:31:40]

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

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

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.

+  }
+
   // TODO(jstritar): We may be able to get rid of this branch by overriding the
   // default extension state to DISABLED when the --disable-extensions flag
   // is set (http://crbug.com/29067).