[Extensions] Log instances of invalid extensions being added

chrome/browser/extensions/extension_service.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/extension_service.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <iterator>
 #include <memory>
 #include <set>
 #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"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_tokenizer.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ skipping 1448 matching lines @@
       to_enable.push_back(extension);
   }
   for (const auto& extension : to_enable) {
     EnableExtension(extension->id());
   }
 
   OnBlacklistUpdated();
 }
 
 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).
   if (!extensions_enabled() && !extension->is_theme() &&
       extension->location() != Manifest::COMPONENT &&
       !Manifest::IsExternalLocation(extension->location()) &&
       disable_flag_exempted_extensions_.count(extension->id()) == 0) {
     return;
   }
 
@@ skipping 1026 matching lines @@
 }
 
 void ExtensionService::OnProfileDestructionStarted() {
   ExtensionIdSet ids_to_unload = registry_->enabled_extensions().GetIDs();
   for (ExtensionIdSet::iterator it = ids_to_unload.begin();
        it != ids_to_unload.end();
        ++it) {
     UnloadExtension(*it, UnloadedExtensionInfo::REASON_PROFILE_SHUTDOWN);
   }
 }