Don't abuse LevelDBObserver interface to pass GetAll result.

Don't abuse LevelDBObserver interface to pass GetAll result.

Instead add a separate (associated) interface just for the purpose of getting
this async response. Also bind using a weak pointer to get this async callback
to make sure the callback isn't actually called after Reset().

Without this fix (or an equivalent) the renderer would DCHECK in the
sanity_check tests, so this also fixes that test to actually give a chance
for async callbacks to run and detect issues like this.

BUG=586194
Committed: https://crrev.com/a3b1e3d5bcc92b7303fdddcbd8994c66a4134e2a
Cr-Commit-Position: refs/heads/master@{#440445}

[Marijn Kruisselbrink, 2016-12-21 18:20:41]

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

[commit-bot: I haz the power, 2016-12-21 18:21:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marijn Kruisselbrink, 2016-12-21 18:53:47]

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

[commit-bot: I haz the power, 2016-12-21 18:54:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marijn Kruisselbrink, 2016-12-21 18:54:56]

Description was changed from

==========
Don't abuse LevelDBObserver interface to pass GetAll result.

Instead add a separate (associated) interface just for the purpose of getting
this async response. Also bind using a weak pointer to get this async callback
to make sure the callback isn't actually called after Reset().

Currently we always immediately create a new map after calling Reset(), but if
that were to change in the future we would have a potential problem where the
GetAll callback could be called incorrectly, if it weren't for this change.

BUG=586194
==========

to

==========
Don't abuse LevelDBObserver interface to pass GetAll result.

Instead add a separate (associated) interface just for the purpose of getting
this async response. Also bind using a weak pointer to get this async callback
to make sure the callback isn't actually called after Reset().

Without this fix (or an equivalent) the renderer would DCHECK in the
sanity_check tests, so this also fixes that test to actually give a chance
for async callbacks to run and detect issues like this.

BUG=586194
==========

[Marijn Kruisselbrink, 2016-12-21 19:01:09]

mek@chromium.org changed reviewers:
+ jam@chromium.org, michaeln@chromium.org

[Marijn Kruisselbrink, 2016-12-21 19:01:09]

A simpler fix for the dcheck failures this is fixing would of course be to just
clear get_all_request_id_ in Reset. But using a request id (which is almost more
a routing ID here), and broadcasting GetAllComplete to all observers when mojo
already has a perfect mechanism to direct a message at a specific target just
feels really wrong to me. Hence this approach.

[commit-bot: I haz the power, 2016-12-21 20:38:42]

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

[commit-bot: I haz the power, 2016-12-21 20:38:42]

Dry run: This issue passed the CQ dry run.

[jam, 2016-12-21 21:24:39]

lgtm

[Marijn Kruisselbrink, 2016-12-21 21:34:31]

mek@chromium.org changed reviewers:
+ dcheng@chromium.org

[Marijn Kruisselbrink, 2016-12-21 21:34:32]

+dcheng for mojom owners

[dcheng, 2016-12-22 07:21:47]

LGTM - it feels a bit awkward, but it makes sense why this is needed.

https://codereview.chromium.org/2593503005/diff/20001/content/browser/leveldb...
File content/browser/leveldb_wrapper_impl.cc (right):

https://codereview.chromium.org/2593503005/diff/20001/content/browser/leveldb...
content/browser/leveldb_wrapper_impl.cc:224:
base::Passed(std::move(complete_callback)), callback));
Nit: base::Passed(&complete_callback)

https://codereview.chromium.org/2593503005/diff/20001/content/renderer/dom_st...
File content/renderer/dom_storage/local_storage_cached_area.cc (right):

https://codereview.chromium.org/2593503005/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.cc:46:
base::WrapUnique(new GetAllCallback(callback)), std::move(request));
Nit: base::MakeUnique<GetAllCallback>(callback)

[Marijn Kruisselbrink, 2016-12-22 17:12:19]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2016-12-22 17:12:19]

https://codereview.chromium.org/2593503005/diff/20001/content/browser/leveldb...
File content/browser/leveldb_wrapper_impl.cc (right):

https://codereview.chromium.org/2593503005/diff/20001/content/browser/leveldb...
content/browser/leveldb_wrapper_impl.cc:224:
base::Passed(std::move(complete_callback)), callback));
On 2016/12/22 at 07:21:47, dcheng wrote:
> Nit: base::Passed(&complete_callback)

I find the std::move version of base::Passed more readable (and of course the
future with OnceCallback and std::move only would be even better. Did you make
any more progress on getting mojo bindings to use OnceCallback?)

https://codereview.chromium.org/2593503005/diff/20001/content/renderer/dom_st...
File content/renderer/dom_storage/local_storage_cached_area.cc (right):

https://codereview.chromium.org/2593503005/diff/20001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.cc:46:
base::WrapUnique(new GetAllCallback(callback)), std::move(request));
On 2016/12/22 at 07:21:47, dcheng wrote:
> Nit: base::MakeUnique<GetAllCallback>(callback)

That would require making the constructor public (which I could do of course,
but the current code seems just as valid style-wise to me).

[commit-bot: I haz the power, 2016-12-22 17:12:40]

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, 2016-12-22 17:18:12]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1482426739093420,
"parent_rev": "9bd362e57e3248e53e76a70ada0e01346106bcbc", "commit_rev":
"ad476cd8eb2f1285380551c460e30ceda1cd3261"}

[commit-bot: I haz the power, 2016-12-22 17:18:42]

Description was changed from

==========
Don't abuse LevelDBObserver interface to pass GetAll result.

Instead add a separate (associated) interface just for the purpose of getting
this async response. Also bind using a weak pointer to get this async callback
to make sure the callback isn't actually called after Reset().

Without this fix (or an equivalent) the renderer would DCHECK in the
sanity_check tests, so this also fixes that test to actually give a chance
for async callbacks to run and detect issues like this.

BUG=586194
==========

to

==========
Don't abuse LevelDBObserver interface to pass GetAll result.

Instead add a separate (associated) interface just for the purpose of getting
this async response. Also bind using a weak pointer to get this async callback
to make sure the callback isn't actually called after Reset().

Without this fix (or an equivalent) the renderer would DCHECK in the
sanity_check tests, so this also fixes that test to actually give a chance
for async callbacks to run and detect issues like this.

BUG=586194

Review-Url: https://codereview.chromium.org/2593503005
==========

[commit-bot: I haz the power, 2016-12-22 17:18:43]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-22 17:20:08]

Description was changed from

==========
Don't abuse LevelDBObserver interface to pass GetAll result.

Instead add a separate (associated) interface just for the purpose of getting
this async response. Also bind using a weak pointer to get this async callback
to make sure the callback isn't actually called after Reset().

Without this fix (or an equivalent) the renderer would DCHECK in the
sanity_check tests, so this also fixes that test to actually give a chance
for async callbacks to run and detect issues like this.

BUG=586194

Review-Url: https://codereview.chromium.org/2593503005
==========

to

==========
Don't abuse LevelDBObserver interface to pass GetAll result.

Instead add a separate (associated) interface just for the purpose of getting
this async response. Also bind using a weak pointer to get this async callback
to make sure the callback isn't actually called after Reset().

Without this fix (or an equivalent) the renderer would DCHECK in the
sanity_check tests, so this also fixes that test to actually give a chance
for async callbacks to run and detect issues like this.

BUG=586194

Committed: https://crrev.com/a3b1e3d5bcc92b7303fdddcbd8994c66a4134e2a
Cr-Commit-Position: refs/heads/master@{#440445}
==========

[commit-bot: I haz the power, 2016-12-22 17:20:09]

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

[dcheng, 2016-12-22 19:08:33]

On 2016/12/22 17:12:19, Marijn Kruisselbrink wrote:
>
https://codereview.chromium.org/2593503005/diff/20001/content/browser/leveldb...
> File content/browser/leveldb_wrapper_impl.cc (right):
> 
>
https://codereview.chromium.org/2593503005/diff/20001/content/browser/leveldb...
> content/browser/leveldb_wrapper_impl.cc:224:
> base::Passed(std::move(complete_callback)), callback));
> On 2016/12/22 at 07:21:47, dcheng wrote:
> > Nit: base::Passed(&complete_callback)
> 
> I find the std::move version of base::Passed more readable (and of course the
> future with OnceCallback and std::move only would be even better. Did you make
> any more progress on getting mojo bindings to use OnceCallback?)

The argument for using & is for consistency. base::Passed(&x) 6-8x more common
than base::Passed(std::move(x)).

OnceCallback is blocked on resolving https://crbug.com/668014 and figuring out:
- where this helper should live. //base doesn't really want it, but it might end
up there anyway
- if it's there, what's the point of a move-only OnceCallback
- in fact, why even rvalue qualify OnceCallback::Run() at that point

Mostly, these just require someone to sit down and think about the implications.
The actual code changes will be fairly mechanical: the Mojo bindings already
allow you to use OnceCallback if you're feeling brave, in fact.

> 
>
https://codereview.chromium.org/2593503005/diff/20001/content/renderer/dom_st...
> File content/renderer/dom_storage/local_storage_cached_area.cc (right):
> 
>
https://codereview.chromium.org/2593503005/diff/20001/content/renderer/dom_st...
> content/renderer/dom_storage/local_storage_cached_area.cc:46:
> base::WrapUnique(new GetAllCallback(callback)), std::move(request));
> On 2016/12/22 at 07:21:47, dcheng wrote:
> > Nit: base::MakeUnique<GetAllCallback>(callback)
> 
> That would require making the constructor public (which I could do of course,
> but the current code seems just as valid style-wise to me).