Added CHECKs for null pointers in ChromeAppSorting.

chrome/browser/extensions/chrome_app_sorting.cc

 // Copyright 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/chrome_app_sorting.h"
 
 #include <algorithm>
 #include <vector>
 
 #include "chrome/browser/chrome_notification_types.h"
@@ skipping 39 matching lines @@
 ChromeAppSorting::ChromeAppSorting()
     : extension_scoped_prefs_(NULL),
       extension_sync_service_(NULL),
       default_ordinals_created_(false) {
 }
 
 ChromeAppSorting::~ChromeAppSorting() {
 }
 
 void ChromeAppSorting::SetExtensionScopedPrefs(ExtensionScopedPrefs* prefs) {
   extension_scoped_prefs_ = prefs;

[not at google - send to devlin, 2015/04/23 18:17:14]

perhaps add 2 extra checks here, that |extension_scoped_prefs_| is null and
|prefs| is not.

[Matt Giuca, 2015/04/24 01:14:14]

I don't want to alter this function. One of my theories is that perhaps this
function (which is deduplicated with 14 other identical functions in the
codebase) is being modified or nop'd out. Adding checks will stop it from being
deduplicated, which may end up "fixing" the problem. I'd rather diagnose it than
fix it by accident.

(Which is why I added the check in ExtensionPrefs right after calling this,
rather than inside the body of this function.)

If the check in ExtensionPrefs fails immediately after calling
SetExtensionScopedPrefs, we'll know that SetExtensionScopedPrefs is bad. Just to
be sure, I've added a CHECK(this) in SetExtensionScopedPrefs to catch the
unlikely event that the constructor is being run on a null pointer.

 }
 
+void ChromeAppSorting::CheckExtensionScopedPrefs() const {
+  CHECK(extension_scoped_prefs_);
+}
+
 void ChromeAppSorting::SetExtensionSyncService(
     ExtensionSyncService* extension_sync_service) {
   extension_sync_service_ = extension_sync_service;
 }
 
 void ChromeAppSorting::Initialize(
     const extensions::ExtensionIdList& extension_ids) {
+  CHECK(extension_scoped_prefs_);

[not at google - send to devlin, 2015/04/23 18:17:14]

For full debugging goodness, perhaps log |extension_ids|. You never know. Of
course, we already get installed-extension data in crash reports, but not those
that will be initialized here.

You may need to copy |extension_ids| onto the stack for them to come up in the
minidump.

[Matt Giuca, 2015/04/24 01:14:14]

The crash reports all have 0 extensions, so I assume that data has not been
loaded yet. Logging the extension IDs would be good. What do you have in mind?

I'm not really familiar with what is and isn't included in crash reports. Do you
mean to VLOG the IDs? (I wasn't aware that would show up in the crash report.)
Or just meant "log" in the sense of "we should try to capture it", such as your
below suggestion.
I'm not sure how to accomplish this exactly. Currently, I can see the vector's
internal array in the crash dump, with all the strings, but the string contents
are not available*. To make it available in the crash dump, I think it would
have to be copied into a local array or vector of char[32]s. That might get
optimized out if it is not actually used. I'm not sure this is worth it (since I
don't suspect that this crash has anything to do with the specific extension IDs
being used). Am I on the right track? Can you suggest any reliable way to get
the extension IDs into the stack trace without adding a huge amount of new code?

*I'm confused about what objects show up in the crash report. The extension IDs
are not accessible (because they are heap-allocated). But also, I would have
assumed that the ChromeAppSorting object is heap-allocated, and yet it shows up
in the crash report. So does the vector. But not the strings pointed at by the
vector. So I am assuming that the minidump includes heap-allocated objects
pointed at directly from the stack, but not transitive.

   InitializePageOrdinalMap(extension_ids);
 
   MigrateAppIndex(extension_ids);
 }
 
 void ChromeAppSorting::CreateOrdinalsIfNecessary(size_t minimum_size) {
   // Create StringOrdinal values as required to ensure |ntp_ordinal_map_| has at
   // least |minimum_size| entries.
   if (ntp_ordinal_map_.empty() && minimum_size > 0)
     ntp_ordinal_map_[syncer::StringOrdinal::CreateInitialOrdinal()];
@@ skipping 383 matching lines @@
       return_value = app_list.rbegin()->first;
     else if (return_type == ChromeAppSorting::MIN_ORDINAL)
       return_value = app_list.begin()->first;
   }
 
   return return_value;
 }
 
 void ChromeAppSorting::InitializePageOrdinalMap(
     const extensions::ExtensionIdList& extension_ids) {
+  // TODO(mgiuca): Added this CHECK to try and diagnose http://crbug.com/476648.
+  // Remove it after the investigation is concluded.
+  CHECK(extension_scoped_prefs_);

[not at google - send to devlin, 2015/04/23 18:17:14]

Ditto

   for (extensions::ExtensionIdList::const_iterator ext_it =
            extension_ids.begin(); ext_it != extension_ids.end(); ++ext_it) {
     AddOrdinalMapping(*ext_it,
                       GetPageOrdinal(*ext_it),
                       GetAppLaunchOrdinal(*ext_it));
 
     // Ensure that the web store app still isn't found in this list, since
     // it is added after this loop.
     DCHECK(*ext_it != extensions::kWebStoreAppId);
     DCHECK(*ext_it != extension_misc::kChromeAppId);
@@ skipping 136 matching lines @@
   for (AppLaunchOrdinalMap::const_iterator it = m.begin(); it != m.end();
        ++it) {
     const std::string& id = it->second;
     if (ntp_hidden_extensions_.count(id) == 0)
       result++;
   }
   return result;
 }
 
 }  // namespace extensions