Turn WorkerLoaderProxy into a threadsafe, ref-counted object.

Turn WorkerLoaderProxy into a threadsafe, ref-counted object.

To avoid relying on the loader client bridge's WorkerLoaderProxy reference
remaining valid until all uses of that bridge object have been finalized,
turn the WorkerLoaderProxy object into a threadsafe, ref-counted object.

Along with that, make the WorkerLoaderProxy implementation more flexible,
allowing the "backend" that actually proxies calls between the worker
and the main thread context to be detached when it is destructed.
This to allow it to be finalized before the WorkerLoaderProxy
instance can be. WorkerLoaderProxyProvider defines that "backend"
interface.

R=kinuko,haraken
BUG=451344
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189549

[kinuko, 2015-02-02 14:56:05]

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2015-02-02 14:56:05]

Hi,

Thanks very much for making this patch.  This patch's bigger but to me this one
(or introducing some synchronization in cancel()) feels more 'right', as the
underlying problem of the issue is (in my understanding) WorkerLoaderProxy gets
deleted without synchronization while it's shared across multiple threads.

On the other hand I feel introducing thread-safe weakptr template is an overkill
only for addressing this case. Could something like below work?

- WorkerLoaderProxy holds WorkerLoaderProxyProvider as a raw ptr and a Mutex
protecting the ptr
- WorkerLoaderProxy also provides a method like willDestroyProvider(), which
clears the ptr in the lock
- WorkerLoaderProxyProvider could have a method getProxy() which returns
PassRefPtr<WorkerLoaderProxy>
- Each impl of WorkerLoaderProxyProvider can create proxy on creation (or when
getProxy() is called for the first time) and hold it as its member variable
(e.g. RefPtr<WorkerLoaderProxy> m_proxy)
- When WorkerLoaderProxyProvider is going to be deleted (i.e. in its dtor) it
can call m_proxy->willDestroyProvider()

[sof, 2015-02-03 21:52:46]

On 2015/02/02 14:56:05, kinuko wrote:
> Hi,
> 
> Thanks very much for making this patch.  This patch's bigger but to me this
one
> (or introducing some synchronization in cancel()) feels more 'right', as the
> underlying problem of the issue is (in my understanding) WorkerLoaderProxy
gets
> deleted without synchronization while it's shared across multiple threads.
> 

Yes, exactly so. We lack a clear&safe way of disabling/removing those
WorkerLoaderProxy references from further main thread tasks' use when finalizing
the worker context it proxies. As mentioned earlier, it's right now sort of
handled in a non-Oilpan setting by destructing the document's
MainThreadTaskRunner, which disables "in flight" tasks from actually running &
touching invalid WorkerLoaderProxy& references. Arguably a bit indirect.

Thanks for the suggested alternative; my sincere apologies for not finding the
time to work on it today -- other things distracted me -- will get to it
tomorrow & follow up further then.

[sof, 2015-02-04 22:50:01]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2015-02-04 22:50:01]

I adopted that weak-like unregistration scheme instead; let me know if that fits
with what you had in mind? (Whenever there is time.)

i.e., I think you're right that such a scheme is simpler compared to the general
abstraction added&used in ps#1.

[kinuko, 2015-02-05 01:41:16]

Thanks, this LGTM! (With two nit comments)

https://codereview.chromium.org/887463003/diff/40001/Source/core/workers/Work...
File Source/core/workers/WorkerLoaderProxy.h (right):

https://codereview.chromium.org/887463003/diff/40001/Source/core/workers/Work...
Source/core/workers/WorkerLoaderProxy.h:79: // no longer be accessed.
Can we make this comment more explicit (and maybe make it a class-level comment)
to note that provider implementor or provider holder must call this before
destructing the provider?

https://codereview.chromium.org/887463003/diff/40001/Source/web/WebEmbeddedWo...
File Source/web/WebEmbeddedWorkerImpl.h (right):

https://codereview.chromium.org/887463003/diff/40001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.h:97: virtual bool
postTaskToWorkerGlobalScope(PassOwnPtr<ExecutionContextTask>) override;
It doesn't seem really necessary to make this change in the class relationship?
I.e. LoaderProxy(Provider)  can be kept as an inner class of this class so that
it doesn't need to expose loader details in the header?

[sof, 2015-02-05 07:58:49]

New patchsets have been uploaded after l-g-t-m from kinuko@chromium.org

[sof, 2015-02-05 08:05:27]

Thanks for the quick review.

https://codereview.chromium.org/887463003/diff/40001/Source/core/workers/Work...
File Source/core/workers/WorkerLoaderProxy.h (right):

https://codereview.chromium.org/887463003/diff/40001/Source/core/workers/Work...
Source/core/workers/WorkerLoaderProxy.h:79: // no longer be accessed.
On 2015/02/05 01:41:16, kinuko wrote:
> Can we make this comment more explicit (and maybe make it a class-level
comment)
> to note that provider implementor or provider holder must call this before
> destructing the provider?

Yes, the comments here were a bit outdated overall. Updated and made the
requirement clear wrt detachProvider().

(If anyone can come up with better naming than "Provider", please change.)

https://codereview.chromium.org/887463003/diff/40001/Source/web/WebEmbeddedWo...
File Source/web/WebEmbeddedWorkerImpl.h (right):

https://codereview.chromium.org/887463003/diff/40001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.h:97: virtual bool
postTaskToWorkerGlobalScope(PassOwnPtr<ExecutionContextTask>) override;
On 2015/02/05 01:41:16, kinuko wrote:
> It doesn't seem really necessary to make this change in the class
relationship?
> I.e. LoaderProxy(Provider)  can be kept as an inner class of this class so
that
> it doesn't need to expose loader details in the header?

The methods are private and the "provider" class is privately derived. It makes
some sense to avoid adding even more indirections via a private class wrapper
object that's additionally owned along with the WorkerLoaderProxy.

I experimented locally with how this would look, but I don't see the benefit, so
won't pursue. Hope that's acceptable.

[kinuko, 2015-02-05 08:58:06]

still lgtm

https://codereview.chromium.org/887463003/diff/40001/Source/web/WebEmbeddedWo...
File Source/web/WebEmbeddedWorkerImpl.h (right):

https://codereview.chromium.org/887463003/diff/40001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.h:97: virtual bool
postTaskToWorkerGlobalScope(PassOwnPtr<ExecutionContextTask>) override;
On 2015/02/05 08:05:27, sof wrote:
> On 2015/02/05 01:41:16, kinuko wrote:
> > It doesn't seem really necessary to make this change in the class
> relationship?
> > I.e. LoaderProxy(Provider)  can be kept as an inner class of this class so
> that
> > it doesn't need to expose loader details in the header?
> 
> The methods are private and the "provider" class is privately derived. It
makes
> some sense to avoid adding even more indirections via a private class wrapper
> object that's additionally owned along with the WorkerLoaderProxy.
> 
> I experimented locally with how this would look, but I don't see the benefit,
so
> won't pursue. Hope that's acceptable.

Ok this works for me too.  I overlooked that now it's derived privately.

[sof, 2015-02-05 09:01:30]

On 2015/02/05 08:58:06, kinuko wrote:
> still lgtm
> 
>
https://codereview.chromium.org/887463003/diff/40001/Source/web/WebEmbeddedWo...
> File Source/web/WebEmbeddedWorkerImpl.h (right):
> 
>
https://codereview.chromium.org/887463003/diff/40001/Source/web/WebEmbeddedWo...
> Source/web/WebEmbeddedWorkerImpl.h:97: virtual bool
> postTaskToWorkerGlobalScope(PassOwnPtr<ExecutionContextTask>) override;
> On 2015/02/05 08:05:27, sof wrote:
> > On 2015/02/05 01:41:16, kinuko wrote:
> > > It doesn't seem really necessary to make this change in the class
> > relationship?
> > > I.e. LoaderProxy(Provider)  can be kept as an inner class of this class so
> > that
> > > it doesn't need to expose loader details in the header?
> > 
> > The methods are private and the "provider" class is privately derived. It
> makes
> > some sense to avoid adding even more indirections via a private class
wrapper
> > object that's additionally owned along with the WorkerLoaderProxy.
> > 
> > I experimented locally with how this would look, but I don't see the
benefit,
> so
> > won't pursue. Hope that's acceptable.
> 
> Ok this works for me too.  I overlooked that now it's derived privately.

Thanks; with that confirmation in place, will try to land.

[sof, 2015-02-05 09:01:38]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-02-05 09:03:14]

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

[commit-bot: I haz the power, 2015-02-05 09:11:12]

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

[commit-bot: I haz the power, 2015-02-05 09:11:14]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/2...)

[kinuko, 2015-02-05 09:15:09]

You'll need owner review for Source/modules/websockets and Source/web..

[sof, 2015-02-05 09:29:50]

On 2015/02/05 09:15:09, kinuko wrote:
> You'll need owner review for Source/modules/websockets and Source/web..

haraken@, tkent@: could you take a look when possible? tia

[haraken, 2015-02-05 09:39:32]

On 2015/02/05 09:29:50, sof wrote:
> On 2015/02/05 09:15:09, kinuko wrote:
> > You'll need owner review for Source/modules/websockets and Source/web..
> 
> haraken@, tkent@: could you take a look when possible? tia

I just scanned. Rubberstamp LGTM based on kinuko-san's LG.

[sof, 2015-02-05 09:43:37]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-02-05 09:45:24]

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

[commit-bot: I haz the power, 2015-02-05 10:31:25]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189549