Use mojo to connect to BackgroundSyncManager object

Use mojo to connect to BackgroundSyncManager object

Removes the BackgroundSyncProviderThreadProxy class, and the manual hopping from workers to the main thread.  The proxy class was passing WebStrings across threads, which aren't thread-safe.

With mojo connections, it lets mojo handle the thread hopping more efficiently, and the mojo structs passed are thread-safe.

Tested using https://jakearchibald.github.io/isserviceworkerready/demos/sync/, as well as new browser tests.

BUG=527601
Committed: https://crrev.com/f993b733666c5c3664889be5c61b65f3f48b222d
Cr-Commit-Position: refs/heads/master@{#354963}

[chasej, 2015-09-28 16:07:42]

chasej@chromium.org changed reviewers:
+ jkarlin@chromium.org

[chasej, 2015-09-28 16:09:03]

jkarlin, please take a look.

[jkarlin, 2015-09-28 20:10:05]

jkarlin@chromium.org changed reviewers:
+ michaeln@chromium.org

[jkarlin, 2015-09-28 20:10:05]

+R michaeln as he is interested in this bug

https://codereview.chromium.org/1358063004/diff/40001/content/child/backgroun...
File content/child/background_sync/background_sync_provider.cc (right):

https://codereview.chromium.org/1358063004/diff/40001/content/child/backgroun...
content/child/background_sync/background_sync_provider.cc:35:
mojo::InterfaceRequest<BackgroundSyncService> request) {
DCHECK that we're on the main thread

https://codereview.chromium.org/1358063004/diff/40001/content/child/backgroun...
File content/child/background_sync/background_sync_provider.h (right):

https://codereview.chromium.org/1358063004/diff/40001/content/child/backgroun...
content/child/background_sync/background_sync_provider.h:23: //
BackgroundSyncManager object in the browser process. Each thread will have
It communicates with the BackgroundSyncManager via the
BackgroundSyncServiceImpl.

https://codereview.chromium.org/1358063004/diff/40001/content/child/backgroun...
content/child/background_sync/background_sync_provider.h:26: // mojo connection
to the BackgroundSyncManager object in the browser process.
Each instance creates a new mojo connection to a new BackgroundSyncServiceImpl.
The ServiceImpl talks to the BackgroundSyncManager.

[jkarlin, 2015-09-28 20:12:51]

https://codereview.chromium.org/1358063004/diff/40001/content/child/backgroun...
File content/child/background_sync/background_sync_provider.h (right):

https://codereview.chromium.org/1358063004/diff/40001/content/child/backgroun...
content/child/background_sync/background_sync_provider.h:73: void
WillStopCurrentWorkerThread() override;
I see how worker threads get killed, thanks to this override. But how does the
main thread's provider get killed?

[michaeln, 2015-10-02 00:13:08]

thnx for cc'ing me

https://codereview.chromium.org/1358063004/diff/40001/content/child/backgroun...
File content/child/background_sync/background_sync_provider.h (right):

https://codereview.chromium.org/1358063004/diff/40001/content/child/backgroun...
content/child/background_sync/background_sync_provider.h:73: void
WillStopCurrentWorkerThread() override;
On 2015/09/28 20:12:51, jkarlin wrote:
> I see how worker threads get killed, thanks to this override. But how does the
> main thread's provider get killed?

For other threadspecific classes like this, the instance for the main thread can
be owned and deleted by the RenderThreadImpl (see IndexedDBDispatcher for
example). In newer classes we usually make a distinction between
GetThreadSpecificInstance() and GetOrCreateThreadSpecificInstance(). The
RenderThreadImpl dtor could test and conditionally delete the main threads
instance. I think we might intentionally leak some of these objects too???

[chasej, 2015-10-08 03:25:30]

On 2015/10/02 00:13:08, michaeln wrote:
> thnx for cc'ing me
> 
>
https://codereview.chromium.org/1358063004/diff/40001/content/child/backgroun...
> File content/child/background_sync/background_sync_provider.h (right):
> 
>
https://codereview.chromium.org/1358063004/diff/40001/content/child/backgroun...
> content/child/background_sync/background_sync_provider.h:73: void
> WillStopCurrentWorkerThread() override;
> On 2015/09/28 20:12:51, jkarlin wrote:
> > I see how worker threads get killed, thanks to this override. But how does
the
> > main thread's provider get killed?
> 
> For other threadspecific classes like this, the instance for the main thread
can
> be owned and deleted by the RenderThreadImpl (see IndexedDBDispatcher for
> example). In newer classes we usually make a distinction between
> GetThreadSpecificInstance() and GetOrCreateThreadSpecificInstance(). The
> RenderThreadImpl dtor could test and conditionally delete the main threads
> instance. I think we might intentionally leak some of these objects too???

I implemented a similar pattern to IndexedDBDispatcher, except in
BlinkPlatformImpl. 
That class previously held the main thread instance for the provider anyway.
This is
probably best seen by looking at the diff between original code and latest
patch.  Ignore
the intermediate patch (#4), as that was an attempt at a different approach,
which was
then removed.

[chasej, 2015-10-08 03:28:54]

https://codereview.chromium.org/1358063004/diff/40001/content/child/backgroun...
File content/child/background_sync/background_sync_provider.cc (right):

https://codereview.chromium.org/1358063004/diff/40001/content/child/backgroun...
content/child/background_sync/background_sync_provider.cc:35:
mojo::InterfaceRequest<BackgroundSyncService> request) {
On 2015/09/28 20:10:05, jkarlin wrote:
> DCHECK that we're on the main thread

I added a DCHECK for ChildThreadImpl::current(), as that was the only obvious
thing to be checked here.  Let me know if you had something else in mind.

https://codereview.chromium.org/1358063004/diff/40001/content/child/backgroun...
File content/child/background_sync/background_sync_provider.h (right):

https://codereview.chromium.org/1358063004/diff/40001/content/child/backgroun...
content/child/background_sync/background_sync_provider.h:23: //
BackgroundSyncManager object in the browser process. Each thread will have
On 2015/09/28 20:10:05, jkarlin wrote:
> It communicates with the BackgroundSyncManager via the
> BackgroundSyncServiceImpl. 

Acknowledged.

https://codereview.chromium.org/1358063004/diff/40001/content/child/backgroun...
content/child/background_sync/background_sync_provider.h:26: // mojo connection
to the BackgroundSyncManager object in the browser process.
On 2015/09/28 20:10:05, jkarlin wrote:
> Each instance creates a new mojo connection to a new
BackgroundSyncServiceImpl.
> The ServiceImpl talks to the BackgroundSyncManager.

Comment updated to be more precise about the behaviour.

[jkarlin, 2015-10-08 15:43:14]

https://codereview.chromium.org/1358063004/diff/100001/content/child/backgrou...
File content/child/background_sync/background_sync_provider.cc (right):

https://codereview.chromium.org/1358063004/diff/100001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:41:
LazyInstance<ThreadLocalPointer<BackgroundSyncProvider>>::Leaky
Why Leaky?

https://codereview.chromium.org/1358063004/diff/100001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:57: bool
on_main_thread = main_thread_task_runner &&
I don't see how main_thread_task_runner can be nullptr. Remove this check and
DCHECK(main_thread_task_runner) at the start of the method instead.

https://codereview.chromium.org/1358063004/diff/100001/content/test/data/back...
File content/test/data/background_sync/background_sync_test_helpers.js (right):

https://codereview.chromium.org/1358063004/diff/100001/content/test/data/back...
content/test/data/background_sync/background_sync_test_helpers.js:113: {action:
'getRegistrationOneShot', tag: tag});
4 space indent for the argument to the function call

[chasej, 2015-10-09 14:58:30]

jkarlin, michaeln please take another look.

https://codereview.chromium.org/1358063004/diff/100001/content/child/backgrou...
File content/child/background_sync/background_sync_provider.cc (right):

https://codereview.chromium.org/1358063004/diff/100001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:41:
LazyInstance<ThreadLocalPointer<BackgroundSyncProvider>>::Leaky
On 2015/10/08 15:43:14, jkarlin wrote:
> Why Leaky?

This was copied from the previous implementation of
BackgroundSyncProviderThreadProxy.  I've removed it as it's not clear why it
would be needed now (or in the previous implementation).  There's hundreds of
usages of LazyInstance<ThreadLocalPointer<>>, many with Leaky, many without. 
Based on a random sampling, I couldn't tell why Leaky is used in some cases, but
not others.

https://codereview.chromium.org/1358063004/diff/100001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:57: bool
on_main_thread = main_thread_task_runner &&
On 2015/10/08 15:43:14, jkarlin wrote:
> I don't see how main_thread_task_runner can be nullptr. Remove this check and
> DCHECK(main_thread_task_runner) at the start of the method instead.

Done. Also reworked the flag variables to more clearly capture the conditions.

[jkarlin, 2015-10-09 15:13:26]

lgtm but see comment

https://codereview.chromium.org/1358063004/diff/100001/content/child/backgrou...
File content/child/background_sync/background_sync_provider.cc (right):

https://codereview.chromium.org/1358063004/diff/100001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:41:
LazyInstance<ThreadLocalPointer<BackgroundSyncProvider>>::Leaky
On 2015/10/09 14:58:29, chasej wrote:
> On 2015/10/08 15:43:14, jkarlin wrote:
> > Why Leaky?
> 
> This was copied from the previous implementation of
> BackgroundSyncProviderThreadProxy.  I've removed it as it's not clear why it
> would be needed now (or in the previous implementation).  There's hundreds of
> usages of LazyInstance<ThreadLocalPointer<>>, many with Leaky, many without. 
> Based on a random sampling, I couldn't tell why Leaky is used in some cases,
but
> not others.

I found this: "NOTE: Both Singleton and base::LazyInstance provide "leaky"
traits to leak the global on shutdown. This is often advisable (except
potentially in library code where the code may be dynamically loaded into
another process's address space or when data needs to be flushed on process
shutdown) in order to not to slow down shutdown. There are valgrind suppressions
for these "leaky" traits." in
https://www.chromium.org/developers/coding-style/important-abstractions-and-d...

Looks like leaky is preferable.

[chasej, 2015-10-09 16:00:07]

On 2015/10/09 15:13:26, jkarlin wrote:
> lgtm but see comment
> 
>
https://codereview.chromium.org/1358063004/diff/100001/content/child/backgrou...
> File content/child/background_sync/background_sync_provider.cc (right):
> 
>
https://codereview.chromium.org/1358063004/diff/100001/content/child/backgrou...
> content/child/background_sync/background_sync_provider.cc:41:
> LazyInstance<ThreadLocalPointer<BackgroundSyncProvider>>::Leaky
> On 2015/10/09 14:58:29, chasej wrote:
> > On 2015/10/08 15:43:14, jkarlin wrote:
> > > Why Leaky?
> > 
> > This was copied from the previous implementation of
> > BackgroundSyncProviderThreadProxy.  I've removed it as it's not clear why it
> > would be needed now (or in the previous implementation).  There's hundreds
of
> > usages of LazyInstance<ThreadLocalPointer<>>, many with Leaky, many without.

> > Based on a random sampling, I couldn't tell why Leaky is used in some cases,
> but
> > not others.
> 
> I found this: "NOTE: Both Singleton and base::LazyInstance provide "leaky"
> traits to leak the global on shutdown. This is often advisable (except
> potentially in library code where the code may be dynamically loaded into
> another process's address space or when data needs to be flushed on process
> shutdown) in order to not to slow down shutdown. There are valgrind
suppressions
> for these "leaky" traits." in
>
https://www.chromium.org/developers/coding-style/important-abstractions-and-d...
> 
> Looks like leaky is preferable.

I've added back the Leaky traits.

[chasej, 2015-10-13 18:18:24]

chasej@chromium.org changed reviewers:
+ jochen@chromium.org

[chasej, 2015-10-13 18:18:25]

michaeln, please take a look. I loosely followed the IndexedDBDispatcher example
you provided for managing the main thread instance.

jochen, please take a look at content/child/blink_platform_impl.*

[michaeln, 2015-10-14 01:24:35]

lgtm 2

[chasej, 2015-10-14 17:21:18]

chasej@chromium.org changed reviewers:
- jochen@chromium.org

[chasej, 2015-10-14 17:24:53]

chasej@chromium.org changed reviewers:
+ davidben@chromium.org

[chasej, 2015-10-14 17:24:53]

davidben, please take a look at content/child/blink_platform_impl.*

[davidben, 2015-10-14 21:52:16]

https://codereview.chromium.org/1358063004/diff/160001/content/child/blink_pl...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/1358063004/diff/160001/content/child/blink_pl...
content/child/blink_platform_impl.cc:461: main_thread_sync_provider_.reset(
This is a scoped_ptr, but the method is GetOrCreateBlah. That seems wrong. What
if it was a 'Get' and you didn't create a new one? (I realize this is hanging
off a singleton, but still.)

https://codereview.chromium.org/1358063004/diff/160001/content/child/blink_pl...
content/child/blink_platform_impl.cc:1217: main_thread_task_runner_.get());
I'm not familiar with Mojo, but why GetOrCreateThreadSpecificInstance and not
main_thread_sync_provider_? It doesn't look like the latter's even used.

[chasej, 2015-10-15 17:40:27]

davidben, please take another look.

For jkarlin, michaeln, the approach for the main thread provider instance has
changed from what you reviewed.

https://codereview.chromium.org/1358063004/diff/160001/content/child/blink_pl...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/1358063004/diff/160001/content/child/blink_pl...
content/child/blink_platform_impl.cc:461: main_thread_sync_provider_.reset(
On 2015/10/14 21:52:16, David Benjamin wrote:
> This is a scoped_ptr, but the method is GetOrCreateBlah. That seems wrong.
What
> if it was a 'Get' and you didn't create a new one? (I realize this is hanging
> off a singleton, but still.)

I see how this approach looks wrong/confusing.  I've addressed it in my reply to
your other comment.

https://codereview.chromium.org/1358063004/diff/160001/content/child/blink_pl...
content/child/blink_platform_impl.cc:1217: main_thread_task_runner_.get());
On 2015/10/14 21:52:16, David Benjamin wrote:
> I'm not familiar with Mojo, but why GetOrCreateThreadSpecificInstance and not
> main_thread_sync_provider_? It doesn't look like the latter's even used.

The goal is to have a provider instance for each thread (main and workers), and
let mojo handle all the thread hopping. The creation/access to the provider goes
through GetOrCreate, as that is similar regardless of calling thread type. 
However, destruction has to be handled differently for the main thread vs
workers. The main_thread_sync_provider_ was used only to ensure the instance for
the main thread was cleaned up.

There are a number of classes that use some variation of the
ThreadSpecificInstance approach.  I've found another class (WebFileSystemImpl),
that uses a more explicit approach to solve this problem.  I've copied that
approach, and instead added DeleteThreadSpecificInstance(), which allows removal
of the main_thread_sync_provider_.  I think this is better because all
creation/deletion is encapsulated in BackgroundSyncProvider, and
BlinkPlatformImpl now uses symmetric methods for creation and deletion.

[davidben, 2015-10-15 18:16:17]

content/child/blink_platform_impl.* lgtm. I didn't look at the other files
carefully since I wasn't asked to.

This approach for lifetimes seems honestly rather backwards to me (thread-local
storage is just another word for global variable) and BlinkPlatformImpl now
becomes even more global, but it seems that ship has sailed. This file is now
full of ThreadSpecificInstances for things.

[jkarlin, 2015-10-15 19:06:21]

lgtm though I preferred the approach from PS9.

[davidben, 2015-10-15 19:11:39]

On 2015/10/15 19:06:21, jkarlin wrote:
> lgtm though I preferred the approach from PS9.

Perhaps mimic BlinkPlatformImpl::permissionClient? So the main thread one is
just explicitly owned by BlinkPlatformImpl with no bells or whistles. Then if
IsMainThread doesn't check out, you can do something that mucks with
thread-locals and is annoyingly global and implicitly destroyed by being a
WorkerObserver and stuff.

[jkarlin, 2015-10-15 20:36:07]

On 2015/10/15 19:11:39, David Benjamin wrote:
> On 2015/10/15 19:06:21, jkarlin wrote:
> > lgtm though I preferred the approach from PS9.
> 
> Perhaps mimic BlinkPlatformImpl::permissionClient? So the main thread one is
> just explicitly owned by BlinkPlatformImpl with no bells or whistles. Then if
> IsMainThread doesn't check out, you can do something that mucks with
> thread-locals and is annoyingly global and implicitly destroyed by being a
> WorkerObserver and stuff.

sgtm

[chasej, 2015-10-16 16:06:49]

On 2015/10/15 20:36:07, jkarlin wrote:
> On 2015/10/15 19:11:39, David Benjamin wrote:
> > On 2015/10/15 19:06:21, jkarlin wrote:
> > > lgtm though I preferred the approach from PS9.
> > 
> > Perhaps mimic BlinkPlatformImpl::permissionClient? So the main thread one is
> > just explicitly owned by BlinkPlatformImpl with no bells or whistles. Then
if
> > IsMainThread doesn't check out, you can do something that mucks with
> > thread-locals and is annoyingly global and implicitly destroyed by being a
> > WorkerObserver and stuff.
> 
> sgtm

I've implemented the suggested approach to have BlinkPlatformImpl explicitly
own/use the main thread provider.

davidben, jkarlin, please review.

[jkarlin, 2015-10-16 16:10:09]

slgtm, thanks!

[jkarlin, 2015-10-19 11:55:26]

I believe that David is out for a bit. I've reviewed the small changes that
David recommended and he previously l-g-t-m'd. I'd say feel free to land.

[chasej, 2015-10-19 13:46:38]

The CQ bit was checked by chasej@chromium.org

[chasej, 2015-10-19 13:46:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org, jkarlin@chromium.org, davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1358063004/#ps220001
(title: "BlinkPlatformImpl explicitly owns main thread provider")

[commit-bot: I haz the power, 2015-10-19 13:46:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1358063004/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1358063004/220001

[commit-bot: I haz the power, 2015-10-19 14:11:43]

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

[commit-bot: I haz the power, 2015-10-19 14:11:44]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[chasej, 2015-10-19 20:20:55]

chasej@chromium.org changed reviewers:
- davidben@chromium.org

[chasej, 2015-10-19 21:04:33]

jkarlin, michaeln, please take a look.

A recent CL from jkarlin (1398633004) broke the browser tests added in this CL. 
I fixed IsSameOriginWindowProviderHost() to avoid crashing when checking
provider hosts (if any hosts had type SERVICE_WORKER_PROVIDER_FOR_CONTROLLER).

[jkarlin, 2015-10-19 22:33:52]

On 2015/10/19 21:04:33, chasej wrote:
> jkarlin, michaeln, please take a look.
> 
> A recent CL from jkarlin (1398633004) broke the browser tests added in this
CL. 
> I fixed IsSameOriginWindowProviderHost() to avoid crashing when checking
> provider hosts (if any hosts had type SERVICE_WORKER_PROVIDER_FOR_CONTROLLER).

Sorry about that! I have a CL up to fix it as well but will delete it since you
have it here.

SLGTM

[michaeln, 2015-10-19 23:28:45]

still lgtm w

[chasej, 2015-10-20 02:47:24]

The CQ bit was checked by chasej@chromium.org

[chasej, 2015-10-20 02:47:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
jkarlin@chromium.org, davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1358063004/#ps260001
(title: "Fix crash in HasWindowProviderHost")

[commit-bot: I haz the power, 2015-10-20 02:47:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1358063004/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1358063004/260001

[commit-bot: I haz the power, 2015-10-20 02:53:02]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-10-20 02:53:47]

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