[sync] Check against nullptr web_data_backend->GetDatabase()

[sync] Check against nullptr web_data_backend->GetDatabase()

If there is a problem with the web_data_backend databaseat startup
(for example, a version incompatibility), GetDatabase() can return nullptr.
This change guards against that.

BUG=719281
R=skym@chromium.org

Review-Url: https://codereview.chromium.org/2867143002
Cr-Commit-Position: refs/heads/master@{#470370}
Committed: https://chromium.googlesource.com/chromium/src/+/4cfcbe6a7b34e0eb45b4e789b5165ffd75ee243b

[Patrick Noland, 2017-05-08 18:53:44]

The CQ bit was checked by pnoland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 18:54:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-08 18:58:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-08 18:58:57]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Patrick Noland, 2017-05-08 19:29:08]

The CQ bit was checked by pnoland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 19:30:07]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-08 21:58:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-08 21:58:50]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Patrick Noland, 2017-05-08 22:01:13]

Sky, PTAL

I'm pretty sure the test failure is unrelated; it's an Android CustomTabActivity
test that was added three hours ago

[skym, 2017-05-08 22:22:13]

lgtm

https://codereview.chromium.org/2867143002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc
(right):

https://codereview.chromium.org/2867143002/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:397: if
(!GetAutofillTable()->GetAllAutofillEntries(&entries)) {
We're still going to crash if the user goes to chrome://sync-internals and the
db didn't init correctly, right?

https://codereview.chromium.org/2867143002/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:410: void
AutocompleteSyncBridge::ActOnLocalChanges(
Presumably this will never get called if the db is down? Have you tested?

https://codereview.chromium.org/2867143002/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:451: auto
batch = base::MakeUnique<syncer::MetadataBatch>();
Why make the batch first?

https://codereview.chromium.org/2867143002/diff/20001/components/autofill/cor...
File
components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc
(right):

https://codereview.chromium.org/2867143002/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:599:
ResetBridge(true);
It's fairly confusing who/what checked the expectation here. Can you add a
comment?

// Processor's destructor will verify an error was reported.

[Patrick Noland, 2017-05-08 22:34:40]

The CQ bit was checked by pnoland@chromium.org to run a CQ dry run

[Patrick Noland, 2017-05-08 22:36:01]

Description was changed from

==========
[sync] Check against nullptr web_data_backend->GetDatabase()

If there is a problem with the web_data_backend databaseat startup
(for example, a version incompatibility), GetDatabase() can return nullptr.
This change guards against that.

BUG=719281

R=skym@chromium.org
==========

to

==========
[sync] Check against nullptr web_data_backend->GetDatabase()

If there is a problem with the web_data_backend databaseat startup
(for example, a version incompatibility), GetDatabase() can return nullptr.
This change guards against that.

BUG=719281

R=skym@chromium.org
==========

[Patrick Noland, 2017-05-08 22:36:01]

pnoland@chromium.org changed reviewers:
+ mathp@chromium.org

[Patrick Noland, 2017-05-08 22:36:33]

+mathp@ for OWNERS review

https://codereview.chromium.org/2867143002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc
(right):

https://codereview.chromium.org/2867143002/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:397: if
(!GetAutofillTable()->GetAllAutofillEntries(&entries)) {
On 2017/05/08 22:22:12, skym wrote:
> We're still going to crash if the user goes to chrome://sync-internals and the
> db didn't init correctly, right?

When I tested this, sync-internals didn't crash. The Autofill type was reported
as "not running."

https://codereview.chromium.org/2867143002/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:410: void
AutocompleteSyncBridge::ActOnLocalChanges(
On 2017/05/08 22:22:12, skym wrote:
> Presumably this will never get called if the db is down? Have you tested?

When I tested, this was never called if the db was down.

https://codereview.chromium.org/2867143002/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:451: auto
batch = base::MakeUnique<syncer::MetadataBatch>();
On 2017/05/08 22:22:13, skym wrote:
> Why make the batch first?

Done.

https://codereview.chromium.org/2867143002/diff/20001/components/autofill/cor...
File
components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc
(right):

https://codereview.chromium.org/2867143002/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:599:
ResetBridge(true);
On 2017/05/08 22:22:13, skym wrote:
> It's fairly confusing who/what checked the expectation here. Can you add a
> comment?
> 
> // Processor's destructor will verify an error was reported.

Done.

[commit-bot: I haz the power, 2017-05-08 22:37:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mathieu, 2017-05-08 23:34:19]

lgtm, please send autofill reviews to rogerm@ from now on

[Patrick Noland, 2017-05-08 23:44:17]

The CQ bit was checked by pnoland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 23:48:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 02:15:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 02:15:16]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Patrick Noland, 2017-05-09 16:41:15]

The CQ bit was checked by pnoland@chromium.org

[Patrick Noland, 2017-05-09 16:41:15]

The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org,
mathp@chromium.org
Link to the patchset: https://codereview.chromium.org/2867143002/#ps60001
(title: "rebase again")

[commit-bot: I haz the power, 2017-05-09 16:42:05]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 17:37:35]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494348075410760,
"parent_rev": "c7b0cf640eee349516d275dad1a00bd393ac8644", "commit_rev":
"4cfcbe6a7b34e0eb45b4e789b5165ffd75ee243b"}

[commit-bot: I haz the power, 2017-05-09 17:37:45]

Description was changed from

==========
[sync] Check against nullptr web_data_backend->GetDatabase()

If there is a problem with the web_data_backend databaseat startup
(for example, a version incompatibility), GetDatabase() can return nullptr.
This change guards against that.

BUG=719281

R=skym@chromium.org
==========

to

==========
[sync] Check against nullptr web_data_backend->GetDatabase()

If there is a problem with the web_data_backend databaseat startup
(for example, a version incompatibility), GetDatabase() can return nullptr.
This change guards against that.

BUG=719281

R=skym@chromium.org

Review-Url: https://codereview.chromium.org/2867143002
Cr-Commit-Position: refs/heads/master@{#470370}
Committed:
https://chromium.googlesource.com/chromium/src/+/4cfcbe6a7b34e0eb45b4e789b516...
==========

[commit-bot: I haz the power, 2017-05-09 17:37:47]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/4cfcbe6a7b34e0eb45b4e789b516...