Fix crash opening App Info dialog on certain apps in Guest Mode.

Fix crash opening App Info dialog on certain apps in Guest Mode.

The App Info dialog was getting a nullptr on apps with the 'fileSystem'
permission in Guest Mode (notably, the Get Help app). Added null checks
to work around the problem.

BUG=457346
Committed: https://crrev.com/6e8c30f2947959b7d9246ff1a4353c3f81fd141c
Cr-Commit-Position: refs/heads/master@{#321275}

[Matt Giuca, 2015-03-16 23:41:19]

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

[Matt Giuca, 2015-03-16 23:41:19]

Please note: I'm not fully sure we want to commit this. We really should
investigate *why* we're getting nulls here, but I think if we want to merge it,
we should do it quickly with this workaround.

Let me know what you think (whether this is an acceptable change, and also
whether we should merge it).

[sashab, 2015-03-16 23:58:01]

To answer your first question (is this an acceptable change): why don't you add
the apps::SavedFilesService as a member variable of AppInfoPermissionsPanel, and
initialize it in the constructor. Then you can remove all the calls that get a
reference to it throughout the class.

If that's not an option, in CreatePermissionsList(), you could change line 246
(the retained file count check) to first check if the service is non-null. Then
you are guaranteed that the service is non-null in all the other callsites.

To answer your second question, whether we should merge it: do we have any idea
why the service might be null? Reillyg@ might be the person to ask. Should we
open the dialog/display the permissions at all if we have no SavedFilesService?
If the answer is yes, then let's keep the change small and try to merge it. If
the answer is no, don't even call CreatePermissionsList() (just leave the
permissions area empty) if the service is NULL and let's find the real cause of
this problem.

[Matt Giuca, 2015-03-17 01:44:13]

On 2015/03/16 23:58:01, sasha_b wrote:
> To answer your first question (is this an acceptable change): why don't you
add
> the apps::SavedFilesService as a member variable of AppInfoPermissionsPanel,
and
> initialize it in the constructor. Then you can remove all the calls that get a
> reference to it throughout the class.

I could do that but I want to make a mergeable CL and that would be an even
bigger change I think.

> If that's not an option, in CreatePermissionsList(), you could change line 246
> (the retained file count check) to first check if the service is non-null.
Then
> you are guaranteed that the service is non-null in all the other callsites.

It's much better to do null checks at the point where you think null could be,
rather than placing the onus on the calling code to check for null. Otherwise,
it's very brittle (anyone could add a call to any of those functions and
re-introduce the crash).

You didn't really answer the question: I meant is null-checking acceptable
versus fixing it so it's never null. But I think I can answer that question now
that I've done more investigating (yes, null check is good).

> To answer your second question, whether we should merge it: do we have any
idea
> why the service might be null?

I've done some investigating and written a follow-up bug
http://crbug.com/467795. The service is null for off-the-record (incognito)
profiles, and I believe that is by design. (The above bug questions whether this
is a good idea, but I think for now we should assume that is the design.) In
fact the only reason it isn't crashing off the apps grid view is that,
bizarrely, that is using a non-OTR profile even in guest mode, which is another
bug I just filed http://crbug.com/467794.

> Should we
> open the dialog/display the permissions at all if we have no
SavedFilesService?

Well we certainly want to still have the app info dialog, but there'll be no
saved files.

> If the answer is yes, then let's keep the change small and try to merge it. If
> the answer is no, don't even call CreatePermissionsList() (just leave the
> permissions area empty) if the service is NULL and let's find the real cause
of
> this problem.

As I said above, I don't want to *not* have null checks on all calls to
SavedFilesService::Get(), if SavedFilesService::Get() can return nullptr. So
I'll go with my current solution, but rephrase the comments.

PTAL.

[Matt Giuca, 2015-03-19 00:09:14]

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

[Matt Giuca, 2015-03-19 00:09:14]

Ping. This needs to be merged into M42.

Adding benwells@ for alternate review.

[sashab, 2015-03-19 00:19:15]

lgtm

[Matt Giuca, 2015-03-19 00:47:17]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2015-03-19 00:48:26]

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

[commit-bot: I haz the power, 2015-03-19 02:42:47]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-19 02:43:58]

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