Add support for threadsafe completion callback factory.

Add support for threadsafe completion callback factory.

This also makes the default be threadsafe. The old factory wasn't threadsafe even to the extent claimed in the header which was causing hangs in plugins

BUG=136284
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=146611

[brettw, 2012-07-10 20:06:43]

trung: everything
dschuff: native_client changes

Note: the proxy file changes are trivial but they make this patch look much
larger than it is.

[Derek Schuff, 2012-07-10 20:19:01]

+jvoung:
It looks like maybe that pnacl-specific refcount class could now just be
replaced with the new generic threadsafe traits?

[brettw, 2012-07-10 20:22:18]

Hmm, I guess this is true. In the proxy I can't do that because of linking
issues, but there I can't depend on ppapi/cpp. I guess the plugin already does
so it "should" work. I'll try.

[brettw, 2012-07-10 20:28:03]

New snap up.

[jvoung (off chromium), 2012-07-10 20:29:50]

On 2012/07/10 20:19:01, dschuff wrote:
> +jvoung:
> It looks like maybe that pnacl-specific refcount class could now just be
> replaced with the new generic threadsafe traits?


Yeah, I think it could use the generic ones.  It shouldn't need to use
nacl-locks (they are the same as the generic ones underneath, modulo some things
to make them more similar across platforms -- maybe the windows critical
sections are recursive, while the pthreads aren't?)

[Derek Schuff, 2012-07-10 20:31:53]

https://chromiumcodereview.appspot.com/10696157/diff/7001/ppapi/native_client...
File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h (right):

https://chromiumcodereview.appspot.com/10696157/diff/7001/ppapi/native_client...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:188: // TODO(sehr):
remove this when file lookup is through ReverseService.
this comment goes with the previous comment that was removed. Maybe instead of
removing it, just say that thread safety is required to support file lookups.

[brettw, 2012-07-10 21:06:09]

https://chromiumcodereview.appspot.com/10696157/diff/7001/ppapi/native_client...
File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h (right):

https://chromiumcodereview.appspot.com/10696157/diff/7001/ppapi/native_client...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h:188: // TODO(sehr):
remove this when file lookup is through ReverseService.
On 2012/07/10 20:31:54, dschuff wrote:
> this comment goes with the previous comment that was removed. Maybe instead of
> removing it, just say that thread safety is required to support file lookups.

Done.

[viettrungluu, 2012-07-10 22:47:58]

I'm going to head over to your desk.

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/cpp/websocket.h
File ppapi/cpp/websocket.h (right):

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/cpp/websocket...
ppapi/cpp/websocket.h:15: // Windows headers will redefine SendMessage.
This is fine, but why is this in this CL?

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/proxy/proxy_c...
File ppapi/proxy/proxy_completion_callback_factory.h (right):

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/proxy/proxy_c...
ppapi/proxy/proxy_completion_callback_factory.h:38: return --ref_;
What I said elsewhere about maybe dchecking ref > 0....

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/compl...
File ppapi/utility/completion_callback_factory.h (right):

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/compl...
ppapi/utility/completion_callback_factory.h:234: PP_DCHECK(object);
Maybe you should have a comment about the non-thread-safety of Initialize()
(i.e., that you shouldn't even think about using the factory until it's been
initialized)?

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/compl...
ppapi/utility/completion_callback_factory.h:257: PP_DCHECK(object_);
While you're here, I believe this dcheck is wholly redundant.

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/compl...
ppapi/utility/completion_callback_factory.h:312: PP_DCHECK(object_);
"

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/compl...
ppapi/utility/completion_callback_factory.h:379: PP_DCHECK(object_);
"

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/compl...
ppapi/utility/completion_callback_factory.h:461: PP_DCHECK(object_);
"

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/compl...
ppapi/utility/completion_callback_factory.h:548: factory_ = NULL;
*** It's not entirely obvious to me why factory_ doesn't need to be protected
... but my head hurts.

I guess this should only be called by the "owning" factory, which protects it
with its lock?

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/compl...
ppapi/utility/completion_callback_factory.h:552: return factory_ ?
factory_->GetObject() : NULL;
But then I worry about this....

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/compl...
ppapi/utility/completion_callback_factory.h:874: T* object_;
Maybe this should have a comment that this is only initialized once (and then
never changed).

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/compl...
ppapi/utility/completion_callback_factory.h:876: typename ThreadTraits::Lock
lock_;
Probably should indicate that this protects back_pointer_.

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/compl...
File ppapi/utility/completion_callback_factory_thread_traits.h (right):

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/compl...
ppapi/utility/completion_callback_factory_thread_traits.h:10: 
You should probably include ppapi/cpp/logging.h.

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/compl...
ppapi/utility/completion_callback_factory_thread_traits.h:23: /// The threadsafe
version of thread traits. Using this class as the "traits"
s/threadsafe/thread-safe/

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/compl...
ppapi/utility/completion_callback_factory_thread_traits.h:34: /// operations
unweildy for this case that we don't actually use that often.
s/unweildy/unwieldy/

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/compl...
ppapi/utility/completion_callback_factory_thread_traits.h:35: /// As a further
optimization, we can add support for this.
s/\.$/ later./ ?

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/compl...
ppapi/utility/completion_callback_factory_thread_traits.h:58: return --ref_;
Maybe it's worth having a PP_DCHECK(ref > 0)?

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/compl...
ppapi/utility/completion_callback_factory_thread_traits.h:64: };
Maybe you should disallow copy/assign?

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/compl...
ppapi/utility/completion_callback_factory_thread_traits.h:113: bool
is_main_thread_;
I'm not entirely convinced that we should even let this be used off the main
thread at all.

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/compl...
ppapi/utility/completion_callback_factory_thread_traits.h:115: };
"

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/compl...
ppapi/utility/completion_callback_factory_thread_traits.h:121: /// asserts that
the called does not recursively lock.
s/called/caller/ ?

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/compl...
ppapi/utility/completion_callback_factory_thread_traits.h:133:
PP_DCHECK(!lock_held_);
I don't think it's documented anywhere that locks mustn't be held on destruction
(and I don't think pp::Lock cares).

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/compl...
ppapi/utility/completion_callback_factory_thread_traits.h:161: class AutoLock {
It feels kind of dumb that you have to repeat the definition of AutoLock here.
Oh well.

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/threa...
File ppapi/utility/threading/lock.cc (right):

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/threa...
ppapi/utility/threading/lock.cc:12: // The second parameter is the spin count,
for short-held locks it avoid the
s/,/:/ (or a semicolon, or a period, but not a comma). Also, s/avoid/avoids/ .

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/threa...
File ppapi/utility/threading/lock.h (right):

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/threa...
ppapi/utility/threading/lock.h:29: /// Most callers should consider using an
AucoLock instead to automatically
s/Auco/Auto

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/threa...
ppapi/utility/threading/lock.h:67: lock_.Acquire();
Indentation.

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/webso...
File ppapi/utility/websocket/websocket_api.cc (right):

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/webso...
ppapi/utility/websocket/websocket_api.cc:16: #ifdef SendMessage
"

[brettw, 2012-07-10 23:12:57]

New snap up

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/cpp/websocket.h
File ppapi/cpp/websocket.h (right):

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/cpp/websocket...
ppapi/cpp/websocket.h:15: // Windows headers will redefine SendMessage.
On 2012/07/10 22:47:58, viettrungluu wrote:
> This is fine, but why is this in this CL?

I included <windows.h> in more places and it exploded.

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/webso...
File ppapi/utility/websocket/websocket_api.cc (right):

https://chromiumcodereview.appspot.com/10696157/diff/6003/ppapi/utility/webso...
ppapi/utility/websocket/websocket_api.cc:16: #ifdef SendMessage
On 2012/07/10 22:47:58, viettrungluu wrote:
> "

"

[viettrungluu, 2012-07-11 00:11:26]

Okay, LGTM w/nit. Gulp.

https://chromiumcodereview.appspot.com/10696157/diff/12002/ppapi/utility/thre...
File ppapi/utility/threading/lock.cc (right):

https://chromiumcodereview.appspot.com/10696157/diff/12002/ppapi/utility/thre...
ppapi/utility/threading/lock.cc:12: // The second parameter is the spin count;
for short-held locks it avoid the
Nit: You missed the s/avoid/avoids/ part.

[jvoung (off chromium), 2012-07-12 18:01:34]

On 2012/07/11 00:11:26, viettrungluu wrote:
> Okay, LGTM w/nit. Gulp.
> 
>
https://chromiumcodereview.appspot.com/10696157/diff/12002/ppapi/utility/thre...
> File ppapi/utility/threading/lock.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10696157/diff/12002/ppapi/utility/thre...
> ppapi/utility/threading/lock.cc:12: // The second parameter is the spin count;
> for short-held locks it avoid the
> Nit: You missed the s/avoid/avoids/ part.

nacl part LGTM, in case you were waiting on that

[kmixter1, 2012-07-30 21:08:59]

https://chromiumcodereview.appspot.com/10696157/diff/12002/ppapi/utility/comp...
File ppapi/utility/completion_callback_factory_thread_traits.h (right):

https://chromiumcodereview.appspot.com/10696157/diff/12002/ppapi/utility/comp...
ppapi/utility/completion_callback_factory_thread_traits.h:121: ///
MStrong>Note:</strong> in Debug mode, it checks that it is either
typo