Deflake PushMessagingBrowserTest.DenyPushPermissionUnregisters & friends

Deflake PushMessagingBrowserTest.DenyPushPermissionUnregisters & friends

PushMessagingServiceImpl::OnContentSettingChanged calls
PushMessagingService::GetSenderId which hops to the IO thread and
performs an asynchronous database lookup before finally calling
PushMessagingService::ClearPushRegistrationID on the UI thread.

This patch de-flakes a bunch of tests which depend on this sequence
of tasks having completed, by adding (and using) a
PushMessagingServiceImpl::SetContentSettingChangedCallbackForTesting
method which allows tests to wait until the sequence of tasks is
complete.

BUG=464710
Committed: https://crrev.com/6d9e37311bba05586e48560f1e762ada24d17b25
Cr-Commit-Position: refs/heads/master@{#320512}

[johnme, 2015-03-10 17:15:53]

johnme@chromium.org changed reviewers:
+ bauerb@chromium.org

[Bernhard Bauer, 2015-03-10 18:03:19]

Urr... The first WaitForGetUserDataRoundTrip() call works, but is kinda hacky
IMO, because it relies on an implementation detail (the fact that changing
content settings triggers GetSenderId(), which internally jumps between
threads). The second call is just wrong, I think. IIUC,
ClearPushRegistrationID() jumps to the database thread and back, but
GetSenderId() will jump to the IO thread instead, so either of those might
return before the other. What I would do instead is to add a callback to
PushMessagingService and call that back instead of
OnClearPushRegistrationServiceWorkerKey (by posting it to the UI thread). You
could name the setter method for the callback ...ForTesting(), so it wouldn't
increase the content API surface in production.

(As a bit of an aside, the explanation of "waiting for an equally long task to
finish" isn't quite correct, as the work on each of the threads happens
sequentially. That sequentiality is what we are using to synchronize things: If
we follow the same sequence of thread jumps as the code under test, the last
callback in such a sequence is guaranteed to run after the code under test has
finished doing its thing.)

https://codereview.chromium.org/996813002/diff/1/chrome/browser/push_messagin...
File chrome/browser/push_messaging/push_messaging_service_impl.h (right):

https://codereview.chromium.org/996813002/diff/1/chrome/browser/push_messagin...
chrome/browser/push_messaging/push_messaging_service_impl.h:92: friend class
PushMessagingBrowserTest;
Why is this necessary?

https://codereview.chromium.org/996813002/diff/40001/chrome/browser/push_mess...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/996813002/diff/40001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_browsertest.cc:332: 0LL /*
service_worker_registration_id */,  // Any SW will do.
Yeah, I was wondering about that :)

[johnme, 2015-03-12 16:17:51]

> The first WaitForGetUserDataRoundTrip() call works, but is kinda hacky
> IMO, because it relies on an implementation detail

I've done a total rewrite, adding callbacks where necessary to guarantee that
the full sequence of tasks has completed before the test resumes. PTAL :)

[Bernhard Bauer, 2015-03-12 17:46:52]

W00t!

https://codereview.chromium.org/996813002/diff/60001/chrome/browser/push_mess...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/996813002/diff/60001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_service_impl.cc:706: ?
base::Bind(&base::DoNothing)
I think I would indent this four more spaces?

https://codereview.chromium.org/996813002/diff/60001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_service_impl.cc:734: static void
UnregisterCallbackToClosure(
Move this to an anonymous namespace instead of making it static?

https://codereview.chromium.org/996813002/diff/60001/chrome/browser/push_mess...
File chrome/browser/push_messaging/push_messaging_service_impl.h (right):

https://codereview.chromium.org/996813002/diff/60001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_service_impl.h:91: void
SetContentSettingChangedCallbackForTesting(
You might want to add a comment here that explains that the callback will be
called at most once. Or, you could just remove the code that resets the
callback, then you don't need the additional calls to this method either :)

[johnme, 2015-03-13 11:56:13]

Addressed Bernhard's review comments - thanks :)

https://codereview.chromium.org/996813002/diff/60001/chrome/browser/push_mess...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/996813002/diff/60001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_service_impl.cc:706: ?
base::Bind(&base::DoNothing)
On 2015/03/12 17:46:51, Bernhard Bauer wrote:
> I think I would indent this four more spaces?

Done.

https://codereview.chromium.org/996813002/diff/60001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_service_impl.cc:734: static void
UnregisterCallbackToClosure(
On 2015/03/12 17:46:51, Bernhard Bauer wrote:
> Move this to an anonymous namespace instead of making it static?

Done.

https://codereview.chromium.org/996813002/diff/60001/chrome/browser/push_mess...
File chrome/browser/push_messaging/push_messaging_service_impl.h (right):

https://codereview.chromium.org/996813002/diff/60001/chrome/browser/push_mess...
chrome/browser/push_messaging/push_messaging_service_impl.h:91: void
SetContentSettingChangedCallbackForTesting(
On 2015/03/12 17:46:51, Bernhard Bauer wrote:
> You might want to add a comment here that explains that the callback will be
> called at most once. Or, you could just remove the code that resets the
> callback, then you don't need the additional calls to this method either :)

Ok, removed the code that resets the callback :)

[Bernhard Bauer, 2015-03-13 12:28:37]

LGTM with a nit. Thanks for indulging me!

https://codereview.chromium.org/996813002/diff/100001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/996813002/diff/100001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:88: static void
UnregisterCallbackToClosure(
And now the static is not necessary anymore :)

[johnme, 2015-03-13 13:12:17]

Addressed Bernhard's nit

https://codereview.chromium.org/996813002/diff/100001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/996813002/diff/100001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:88: static void
UnregisterCallbackToClosure(
On 2015/03/13 12:28:37, Bernhard Bauer wrote:
> And now the static is not necessary anymore :)

Darn! Done.

[johnme, 2015-03-13 13:14:04]

johnme@chromium.org changed reviewers:
+ avi@chromium.org

[johnme, 2015-03-13 13:14:04]

avi@chromium.org: Please review changes in content/

[Avi (use Gerrit), 2015-03-13 13:58:05]

Can you please use MessageLoopRunner rather than playing with RunLoops directly?

https://codereview.chromium.org/996813002/diff/120001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/996813002/diff/120001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:108: base::RunLoop
run_loop_;
MessageLoopRunner.

[johnme, 2015-03-13 14:21:29]

Switched from RunLoop to MessageLoopRunner, as suggested - thanks.

https://codereview.chromium.org/996813002/diff/120001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/996813002/diff/120001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:108: base::RunLoop
run_loop_;
On 2015/03/13 13:58:04, Avi wrote:
> MessageLoopRunner.

Done.

[Avi (use Gerrit), 2015-03-13 15:14:52]

Thanks! LGTM

[johnme, 2015-03-13 15:52:32]

The CQ bit was checked by johnme@chromium.org

[johnme, 2015-03-13 15:52:33]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/996813002/#ps160001
(title: "Use scoped_refptr")

[commit-bot: I haz the power, 2015-03-13 15:52:53]

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

[commit-bot: I haz the power, 2015-03-13 16:16:04]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-03-13 16:16:53]

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