Protect against NULL PasswordStore

Protect against NULL PasswordStore

BUG=chromium-os:13799
TEST=See issue

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86459

[stevenjb, 2011-05-18 02:45:44]

PasswordManagerHandler is crashing when
web_ui_->GetProfile()->GetPasswordStore() returns NULL, which can happen when
ProfileImpl::CreatePasswordStore() fails to initialize LoginDatabase(). Our best
guess is that this was happening because of a corrupted database file. This will
cause PasswordManagerHandler to fail to function correctly (and we should handle
that better), but at minimum we should avoid crashing when that happens.

[Sheridan Rawlins, 2011-05-18 04:09:26]

If this is a "best guess" can we get NOTREACHED()'s in there so that if it does
recur on debug build it can be addressed.

On 2011/05/18 02:45:44, Steven Bennetts wrote:
> PasswordManagerHandler is crashing when
> web_ui_->GetProfile()->GetPasswordStore() returns NULL, which can happen when
> ProfileImpl::CreatePasswordStore() fails to initialize LoginDatabase(). Our
best
> guess is that this was happening because of a corrupted database file. This
will
> cause PasswordManagerHandler to fail to function correctly (and we should
handle
> that better), but at minimum we should avoid crashing when that happens.

[stevenjb, 2011-05-18 17:41:43]

We know that this is happening sometimes, but agreed that adding NOTREACHED
could be helpful for debugging, done.

[James Hawkins, 2011-05-18 17:52:27]

http://codereview.chromium.org/7031038/diff/5001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/password_manager_handler.cc (right):

http://codereview.chromium.org/7031038/diff/5001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/password_manager_handler.cc:83: NOTREACHED();
I'm not a fan of the NOTREACHEDs. This will not hit in the wild (it's
essentially a DCHECK), so it's unlikely we'll get any data from it. We've
previously said there are reasons why the password store may not exist (likely
to corrupt profiles), so handling this as a potential case is the right thing to
do (and not pretend like it's unexpected).

[stevenjb, 2011-05-20 01:18:57]

After consideration I agree with James. We already LOG(ERROR) when we fail to
init the login database in ProfileImpl::CreatePasswordStore(). This is a
scenario that PasswordManagerHandler needs to handle gracefully. A user friendly
message might be nice, but not crashing is at least a step in the right
direction.

I also added a comment to the declaration of GetPasswordStore, audited other
uses cases for NULL checks, and added one other fix in the test automation code.

[Sheridan Rawlins, 2011-05-20 07:28:03]

Ok, this makes sense.  

So handling this gracefully is definitely a step in the right direction.  What
about the next step (possibly as a followup bug)?  Would any messaging to the
user help them diagnose for fix this?  If the database is corrupt - can they
repair it somehow manually or will their passwords forever be broken?

I defer to the DOMUI Settings contributors James or Stuart for LG and value of
notifying user of errors.

On 2011/05/20 01:18:57, Steven Bennetts wrote:
> After consideration I agree with James. We already LOG(ERROR) when we fail to
> init the login database in ProfileImpl::CreatePasswordStore(). This is a
> scenario that PasswordManagerHandler needs to handle gracefully. A user
friendly
> message might be nice, but not crashing is at least a step in the right
> direction.
> 
> I also added a comment to the declaration of GetPasswordStore, audited other
> uses cases for NULL checks, and added one other fix in the test automation
code.

[James Hawkins, 2011-05-20 22:28:18]

LGTM

http://codereview.chromium.org/7031038/diff/9002/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/7031038/diff/9002/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider.cc:3610:
reply.SendError("Unable to get password store.");
This is likely more useful as LOG(ERROR), here and elsewhere.

[stuartmorgan, 2011-05-23 16:24:12]

LGTM