PPAPI: Make blocking completion callbacks work.

PPAPI: Make blocking completion callbacks work.

This also makes scoped_refptr<TrackedCallback> the "new" way to pass completion callbacks in an API. This allows the Enter object to handle checking for blocking callbacks on the main thread to report error, and blocking if on the background thread. This way, interfaces don't have to write any special cases for blocking callbacks.

When built with enable_pepper_threading=1 locally, URLLoader tests all pass for blocking completion callbacks. I haven't updated all tests yet.

BUG=92909
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=143806

[dmichael (off chromium), 2012-04-18 22:26:33]

I still am going to make a clean-up pass, since I'm sure I left some obvious
mistakes. But if you get tired of administrative junk and want to read code,
you're welcome to start reviewing now.

[dmichael (off chromium), 2012-04-19 20:02:00]

Okay, I think it's fully ready to go now.

Sorry for the hugeness. Most of it is pretty straightforward, and fortunately I
was able to delete code doing it this way.

http://codereview.chromium.org/10081020/diff/10002/ppapi/proxy/ppb_tcp_server...
File ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc (left):

http://codereview.chromium.org/10081020/diff/10002/ppapi/proxy/ppb_tcp_server...
ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc:167: EnterTCPServerSocket
enter(socket_resource, true);
This is a slightly unrelated change, but it was double-locking before if
enable-pepper-threading=1, which puts a damper on my local testing.

http://codereview.chromium.org/10081020/diff/10002/ppapi/tests/test_case.h
File ppapi/tests/test_case.h (left):

http://codereview.chromium.org/10081020/diff/10002/ppapi/tests/test_case.h#ol...
ppapi/tests/test_case.h:210: if (MatchesFilter(#name"ForceAsync", test_filter))
{ \
Note the first parameter used to say '#name"ForceAsync"', meaning that tests
would have to be listed both ways in ppapi_uitest and be run separately. We
weren't doing that, so URLLoader "ForceAsync" tests weren't actually running...
it seemed silly in retrospect.

Now, when you have this in your test:
RUN_TEST_FORCE_ASYNC_AND_NOT(Bar, filter);
and this in ppapi_uitests.cc:
TEST_PPAPI_IN_PROCESS(Foo_Bar);
We'll run Foo_Bar and Foo_BarAsync both in sequence without restarting
browser_tests.

http://codereview.chromium.org/10081020/diff/10002/ppapi/tests/test_utils.cc
File ppapi/tests/test_utils.cc (left):

http://codereview.chromium.org/10081020/diff/10002/ppapi/tests/test_utils.cc#...
ppapi/tests/test_utils.cc:100: instance_(instance) {
I had erroneously gotten rid of the implementation of run_count_ in a previous
CL but not the interface...  somehow this is not causing flakiness.

http://codereview.chromium.org/10081020/diff/10002/ppapi/thunk/enter.h
File ppapi/thunk/enter.h (left):

http://codereview.chromium.org/10081020/diff/10002/ppapi/thunk/enter.h#oldcod...
ppapi/thunk/enter.h:263: };
I needed EnterInstanceNoLock to make AudioConfigShared work right (it was
double-locking).

http://codereview.chromium.org/10081020/diff/10002/ppapi/thunk/ppb_file_io_tr...
File ppapi/thunk/ppb_file_io_trusted_thunk.cc (left):

http://codereview.chromium.org/10081020/diff/10002/ppapi/thunk/ppb_file_io_tr...
ppapi/thunk/ppb_file_io_trusted_thunk.cc:24: return
enter.SetResult(enter.object()->GetOSFileDescriptor());
With no callback, it doesn't make sense to call SetResult (I added a DCHECK just
to make things clearer, since SetResult is complicated enough without this
support, I'd rather just not allow calls to SetResult when it doesn't really do
anything.

[brettw, 2012-04-20 18:24:29]

http://codereview.chromium.org/10081020/diff/12001/ppapi/shared_impl/api_call...
File ppapi/shared_impl/api_callback_type.h (right):

http://codereview.chromium.org/10081020/diff/12001/ppapi/shared_impl/api_call...
ppapi/shared_impl/api_callback_type.h:13: typedef scoped_refptr<TrackedCallback>
ApiCallbackType;
Personally, I find using this to be less clear than just referencing
scoped_refptr. The scoped_refptr isn't that long and this is both an extra file
and requires that I look up the typedef to figure out what the type is. It feels
like somebody reading this shouldn't have to look under so many rocks to figure
out what's happening.

http://codereview.chromium.org/10081020/diff/12001/ppapi/thunk/enter.h
File ppapi/thunk/enter.h (right):

http://codereview.chromium.org/10081020/diff/12001/ppapi/thunk/enter.h#newcod...
ppapi/thunk/enter.h:162: Resource* resource_;
I'm worried about this. We do a bunch of stuff on destruction of the enter
objects, and the resource might actually be gone by then. I don't see any
obvious problems in this patch, but it seems like something that is easy to
cause bugs in the future.

Using this during initialization is OK. Maybe we should be passing the Resource*
as an argument to the init functions we need (like we did with the PP_Resource)
and not store it?

http://codereview.chromium.org/10081020/diff/12001/ppapi/thunk/enter.h#newcod...
ppapi/thunk/enter.h:206: SetStateForCallbackError(report_error);
I think you should just call this from the beginning of the
SetStateForFunction/ResourceError functions. I separated those out to keep the
templateized code minimal, but we needed slightly different implementatnions for
EnterResource vs. EnterFunction. So I think this would go in the common
non-templaized error handling code.

[dmichael (off chromium), 2012-05-16 22:18:29]

PTAL when you're bored...  not high priority.

http://codereview.chromium.org/10081020/diff/12001/ppapi/shared_impl/api_call...
File ppapi/shared_impl/api_callback_type.h (right):

http://codereview.chromium.org/10081020/diff/12001/ppapi/shared_impl/api_call...
ppapi/shared_impl/api_callback_type.h:13: typedef scoped_refptr<TrackedCallback>
ApiCallbackType;
On 2012/04/20 18:24:29, brettw wrote:
> Personally, I find using this to be less clear than just referencing
> scoped_refptr. The scoped_refptr isn't that long and this is both an extra
file
> and requires that I look up the typedef to figure out what the type is. It
feels
> like somebody reading this shouldn't have to look under so many rocks to
figure
> out what's happening.
Yeah, I agree. I was:
(1) afraid I'd have to change it (if this didn't work, or we didn't want a
dependence from ppapi/thunk to base for some reason, or we wanted to pass by
const-ref, etc).
(2) Too lazy to want to reformat that much code if I didn't have to :-)

Done.

http://codereview.chromium.org/10081020/diff/12001/ppapi/thunk/enter.h
File ppapi/thunk/enter.h (right):

http://codereview.chromium.org/10081020/diff/12001/ppapi/thunk/enter.h#newcod...
ppapi/thunk/enter.h:162: Resource* resource_;
On 2012/04/20 18:24:29, brettw wrote:
> I'm worried about this. We do a bunch of stuff on destruction of the enter
> objects, and the resource might actually be gone by then. I don't see any
> obvious problems in this patch, but it seems like something that is easy to
> cause bugs in the future.
> 
> Using this during initialization is OK. Maybe we should be passing the
Resource*
> as an argument to the init functions we need (like we did with the
PP_Resource)
> and not store it?
Note that EnterResource already stores it and exposes it. This just moves it up
the hierarchy (though it does make it accessible to SetResult, where most of the
behavior is). I took out the smarts in the destructor, FWIW (the reasoning is
that all thunks that use callbacks need to do SetResult, so we should be able to
make sure the callback is handled properly in SetResult and prior to
destruction, since SetResult should always be called for operations that take a
callback.)

The main motivation for moving it up the hierarchy is because (a) I need the
Resource* for the TrackedCallback, and (b) I don't want to do the PP_Resource
lookup twice. I haven't thought of a nice way to get the Resource* passed to the
EnterBase constructor *and* the Init function. But I can think about it some
more. We can also consider something like a slight optimization for Resource
lookup, where we store the last ID used and a Resource weak ptr. Then I wouldn't
need to care about doing 2 lookups on every resource thunk. Such an optimization
might actually help in typical plugin execution as well...

http://codereview.chromium.org/10081020/diff/12001/ppapi/thunk/enter.h#newcod...
ppapi/thunk/enter.h:206: SetStateForCallbackError(report_error);
On 2012/04/20 18:24:29, brettw wrote:
> I think you should just call this from the beginning of the
> SetStateForFunction/ResourceError functions. I separated those out to keep the
> templateized code minimal, but we needed slightly different implementatnions
for
> EnterResource vs. EnterFunction. So I think this would go in the common
> non-templaized error handling code.

Done.

[brettw, 2012-05-20 17:46:46]

http://codereview.chromium.org/10081020/diff/12001/ppapi/thunk/enter.h
File ppapi/thunk/enter.h (right):

http://codereview.chromium.org/10081020/diff/12001/ppapi/thunk/enter.h#newcod...
ppapi/thunk/enter.h:162: Resource* resource_;
Can you put a big scary warning here not to use it after any Pepper function has
been invoked since that can cause the object to be released. I'd also like a
warning at the top of SetResult that you can't use the resource_ member because
it may have been destroyed.

http://codereview.chromium.org/10081020/diff/17285/ppapi/thunk/enter.cc
File ppapi/thunk/enter.cc (right):

http://codereview.chromium.org/10081020/diff/17285/ppapi/thunk/enter.cc#newco...
ppapi/thunk/enter.cc:61: // It doesn't make sense to call SetResult if there is
no callback.
I don't see why you "shouldn't" call this when there's no callback. Maybe just
remove this assertion?

http://codereview.chromium.org/10081020/diff/17285/ppapi/thunk/enter.cc#newco...
ppapi/thunk/enter.cc:128: SetStateForCallbackError(report_error);
This function call should return early on error. If the resource is invalid then
it will overwrite the error. That's probably OK in practice, but seems weird.

http://codereview.chromium.org/10081020/diff/17285/ppapi/thunk/enter.h
File ppapi/thunk/enter.h (right):

http://codereview.chromium.org/10081020/diff/17285/ppapi/thunk/enter.h#newcode68
ppapi/thunk/enter.h:68: static void AssertLockHeld() {
It would be nice to make this non-inline & remove the lock.h include in this
file. How about making this a freestanding function in the subtle namespace that
the class calls?

http://codereview.chromium.org/10081020/diff/17285/ppapi/thunk/enter.h#newcod...
ppapi/thunk/enter.h:279: 
Got extra line.

http://codereview.chromium.org/10081020/diff/17285/ppapi/thunk/ppb_graphics_2...
File ppapi/thunk/ppb_graphics_2d_api.h (right):

http://codereview.chromium.org/10081020/diff/17285/ppapi/thunk/ppb_graphics_2...
ppapi/thunk/ppb_graphics_2d_api.h:30: virtual int32_t
Flush(scoped_refptr<TrackedCallback> callback) = 0;
I hate to bring this up, but is it necessary to pass these as scoped_refptrs as
input args? This will cause an extra threadsafe addref + release for every call
and I think normally we just pass as normal poitners. If the callee wants to
keep a ref, they're welcome to, but the caller will have to keep a ref no matter
what.

[dmichael (off chromium), 2012-05-22 18:08:38]

A couple of notes and a question before I dive in to updating the patch.

http://codereview.chromium.org/10081020/diff/12001/ppapi/thunk/enter.h
File ppapi/thunk/enter.h (right):

http://codereview.chromium.org/10081020/diff/12001/ppapi/thunk/enter.h#newcod...
ppapi/thunk/enter.h:162: Resource* resource_;
On 2012/05/20 17:46:46, brettw wrote:
> Can you put a big scary warning here not to use it after any Pepper function
has
> been invoked since that can cause the object to be released. I'd also like a
> warning at the top of SetResult that you can't use the resource_ member
because
> it may have been destroyed.
Will do.

http://codereview.chromium.org/10081020/diff/17285/ppapi/thunk/enter.cc
File ppapi/thunk/enter.cc (right):

http://codereview.chromium.org/10081020/diff/17285/ppapi/thunk/enter.cc#newco...
ppapi/thunk/enter.cc:61: // It doesn't make sense to call SetResult if there is
no callback.
On 2012/05/20 17:46:46, brettw wrote:
> I don't see why you "shouldn't" call this when there's no callback. Maybe just
> remove this assertion?
I didn't really want to have to code for that case and almost just had:
DCHECK(callback_);
But I thought I should have somewhat reasonable behavior in that case. IMHO it's
not clear what it means to call SetResult with no callback; what I have here is
probably appropriate, but maybe a developer trying to do this would have a
different intent. So I like the assertion.

http://codereview.chromium.org/10081020/diff/17285/ppapi/thunk/enter.cc#newco...
ppapi/thunk/enter.cc:128: SetStateForCallbackError(report_error);
On 2012/05/20 17:46:46, brettw wrote:
> This function call should return early on error. If the resource is invalid
then
> it will overwrite the error. That's probably OK in practice, but seems weird.
My intent was to have both log messages come out, but return
PP_ERROR_BADRESOURCE. I'll update the documentation to reflect that.

http://codereview.chromium.org/10081020/diff/17285/ppapi/thunk/ppb_graphics_2...
File ppapi/thunk/ppb_graphics_2d_api.h (right):

http://codereview.chromium.org/10081020/diff/17285/ppapi/thunk/ppb_graphics_2...
ppapi/thunk/ppb_graphics_2d_api.h:30: virtual int32_t
Flush(scoped_refptr<TrackedCallback> callback) = 0;
On 2012/05/20 17:46:46, brettw wrote:
> I hate to bring this up, but is it necessary to pass these as scoped_refptrs
as
> input args? This will cause an extra threadsafe addref + release for every
call
> and I think normally we just pass as normal poitners. If the callee wants to
> keep a ref, they're welcome to, but the caller will have to keep a ref no
matter
> what.
Aw, this is why I started with a typedef :-p

I like using "scoped_refptr" in the signature to make it clear that it's a
ref-counted object (otherwise ownership is really unclear). And I'd ultimately
like to make scoped_refptr not automatically convert to a raw pointer. How would
you feel about passing by const-ref?

[brettw, 2012-06-01 21:11:31]

http://codereview.chromium.org/10081020/diff/17285/ppapi/thunk/enter.cc
File ppapi/thunk/enter.cc (right):

http://codereview.chromium.org/10081020/diff/17285/ppapi/thunk/enter.cc#newco...
ppapi/thunk/enter.cc:128: SetStateForCallbackError(report_error);
On 2012/05/22 18:08:39, dmichael wrote:
> On 2012/05/20 17:46:46, brettw wrote:
> > This function call should return early on error. If the resource is invalid
> then
> > it will overwrite the error. That's probably OK in practice, but seems
weird.
> My intent was to have both log messages come out, but return
> PP_ERROR_BADRESOURCE. I'll update the documentation to reflect that.

Did you update the documentation? I don't see anything.

[dmichael (off chromium), 2012-06-22 18:14:04]

https://chromiumcodereview.appspot.com/10081020/diff/17285/ppapi/thunk/enter.h
File ppapi/thunk/enter.h (right):

https://chromiumcodereview.appspot.com/10081020/diff/17285/ppapi/thunk/enter....
ppapi/thunk/enter.h:68: static void AssertLockHeld() {
On 2012/05/20 17:46:46, brettw wrote:
> It would be nice to make this non-inline & remove the lock.h include in this
> file. How about making this a freestanding function in the subtle namespace
that
> the class calls?
Good idea, done.

https://chromiumcodereview.appspot.com/10081020/diff/17285/ppapi/thunk/enter....
ppapi/thunk/enter.h:279: 
On 2012/05/20 17:46:46, brettw wrote:
> Got extra line.

Done.

https://chromiumcodereview.appspot.com/10081020/diff/17285/ppapi/thunk/ppb_gr...
File ppapi/thunk/ppb_graphics_2d_api.h (right):

https://chromiumcodereview.appspot.com/10081020/diff/17285/ppapi/thunk/ppb_gr...
ppapi/thunk/ppb_graphics_2d_api.h:30: virtual int32_t
Flush(scoped_refptr<TrackedCallback> callback) = 0;
On 2012/05/22 18:08:39, dmichael wrote:
> On 2012/05/20 17:46:46, brettw wrote:
> > I hate to bring this up, but is it necessary to pass these as scoped_refptrs
> as
> > input args? This will cause an extra threadsafe addref + release for every
> call
> > and I think normally we just pass as normal poitners. If the callee wants to
> > keep a ref, they're welcome to, but the caller will have to keep a ref no
> matter
> > what.
> Aw, this is why I started with a typedef :-p
> 
> I like using "scoped_refptr" in the signature to make it clear that it's a
> ref-counted object (otherwise ownership is really unclear). And I'd ultimately
> like to make scoped_refptr not automatically convert to a raw pointer. How
would
> you feel about passing by const-ref?
^^^^^ Just wanted to point this comment out again, since it's been forever...  I
can change to passing by const-ref to avoid the extra ref-counting, if you like.
Otherwise, I'd prefer to leave it be.

[brettw, 2012-06-22 19:12:34]

lgtm

[commit-bot: I haz the power, 2012-06-23 02:05:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/10081020/52002

[commit-bot: I haz the power, 2012-06-23 04:31:49]

Try job failure for 10081020-52002 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-06-23 12:43:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/10081020/52002

[commit-bot: I haz the power, 2012-06-23 14:27:45]

Change committed as 143806