Added CHECKs for null pointers in ChromeAppSorting.

Added CHECKs for null pointers in ChromeAppSorting.

These are designed to give us more information about production-only
crashes occurring in ChromeAppSorting. These checks should help narrow
down the times at which extension_scoped_prefs_ is null.

Added TODOs to remove most of the checks later (although I plan to leave
one in as a safeguard).

BUG=476648
Committed: https://crrev.com/927270474a2720c30ee0bef9d19e25267344da36
Cr-Commit-Position: refs/heads/master@{#326746}

[Matt Giuca, 2015-04-23 08:56:53]

mgiuca@chromium.org changed reviewers:
+ kalman@chromium.org

[Matt Giuca, 2015-04-23 08:56:54]

I'm adding CHECKs to help diagnose this bug. I will add more details on the bug
(from comment #35 onwards).

[Matt Giuca, 2015-04-23 08:57:56]

PS. This is intended to be merged to M42, so we need to be *very* careful that
it does not checkfail on startup for normal users (i.e., users who are not
already experiencing the crash). Please review with that intention in mind.

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

lgtm

https://codereview.chromium.org/1055453007/diff/1/chrome/browser/extensions/c...
File chrome/browser/extensions/chrome_app_sorting.cc (right):

https://codereview.chromium.org/1055453007/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_app_sorting.cc:60: extension_scoped_prefs_ =
prefs;
perhaps add 2 extra checks here, that |extension_scoped_prefs_| is null and
|prefs| is not.

https://codereview.chromium.org/1055453007/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_app_sorting.cc:74:
CHECK(extension_scoped_prefs_);
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.

https://codereview.chromium.org/1055453007/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_app_sorting.cc:480:
CHECK(extension_scoped_prefs_);
Ditto

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

https://codereview.chromium.org/1055453007/diff/1/chrome/browser/extensions/c...
File chrome/browser/extensions/chrome_app_sorting.cc (right):

https://codereview.chromium.org/1055453007/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_app_sorting.cc:60: extension_scoped_prefs_ =
prefs;
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.

https://codereview.chromium.org/1055453007/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_app_sorting.cc:74:
CHECK(extension_scoped_prefs_);
On 2015/04/23 18:17:14, kalman wrote:
> 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.

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.

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

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.

[Matt Giuca, 2015-04-24 03:55:27]

I didn't hear back from you, and I wanted to commit this today, so I'm just
going to go ahead. If you have any thoughts about logging the extension IDs,
that can be done in a follow-up CL.

[Matt Giuca, 2015-04-24 03:55:31]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2015-04-24 03:55:32]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1055453007/#ps20001
(title: "Added CHECK(this) just to make sure that ExtensionPrefs is non-null.")

[commit-bot: I haz the power, 2015-04-24 03:55:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1055453007/20001

[commit-bot: I haz the power, 2015-04-24 05:59:37]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-24 06:00:20]

Patchset 2 (id:??) landed as
https://crrev.com/927270474a2720c30ee0bef9d19e25267344da36
Cr-Commit-Position: refs/heads/master@{#326746}