Enable pepper threading by default.

Enable pepper threading by default.

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

[dmichael (off chromium), 2012-11-05 17:45:46]

lgtm

It shouldn't be hard to add something to about:flags for this, if we need to
make it easy to turn off at run-time. Let me know if you want me to do that.

[brettw, 2012-11-05 20:18:29]

PTAL. I had to fix a few tests. One was a recursive lock. The other was a test
that's only run with threading enabled, and I broke it when I added the
permissions check a few months ago and didn't notice.

[dmichael (off chromium), 2012-11-05 20:27:11]

lgtm

[brettw, 2012-11-06 00:56:44]

I added another lock in the testing proxy which I *think* is right. It would be
nice if you could double-check this.

This caused the printing test to fail since we did a nested message loop and
we'd run the callback for getting the print settings outside of the lock.

[dmichael (off chromium), 2012-11-06 03:00:02]

http://codereview.chromium.org/11361075/diff/13001/ppapi/proxy/ppb_testing_pr...
File ppapi/proxy/ppb_testing_proxy.cc (right):

http://codereview.chromium.org/11361075/diff/13001/ppapi/proxy/ppb_testing_pr...
ppapi/proxy/ppb_testing_proxy.cc:57: ProxyAutoLock lock;
No, you don't want the lock to be acquired at this point. All the other code
assumes it is acquired right when you enter the proxy. E.g., when a message
comes in to PluginDispatcher, or if you PostTask, you wrap the task in
RunWhileLocked (which locks around the Closure). This will probably break other
tests that use RunMessageLoop; probably with a deadlock error (i.e., trying to
lock from the same thread that holds the lock already). We need to instead
figure out why the printing code isn't locking and fix it there.

[dmichael (off chromium), 2012-11-06 03:12:19]

http://codereview.chromium.org/11361075/diff/13001/ppapi/proxy/ppb_testing_pr...
File ppapi/proxy/ppb_testing_proxy.cc (right):

http://codereview.chromium.org/11361075/diff/13001/ppapi/proxy/ppb_testing_pr...
ppapi/proxy/ppb_testing_proxy.cc:57: ProxyAutoLock lock;
On 2012/11/06 03:00:02, dmichael wrote:
> No, you don't want the lock to be acquired at this point. All the other code
> assumes it is acquired right when you enter the proxy. E.g., when a message
> comes in to PluginDispatcher, or if you PostTask, you wrap the task in
> RunWhileLocked (which locks around the Closure). This will probably break
other
> tests that use RunMessageLoop; probably with a deadlock error (i.e., trying to
> lock from the same thread that holds the lock already). We need to instead
> figure out why the printing code isn't locking and fix it there.

I've looked around a little, and it hasn't jumped out at me where we might be
failing to acquire the lock. But I don't understand the new resource stuff yet.
Are those messages coming in on a different channel and so not passing through
PluginDispatcher::OnMessageReceived, where we acquire the lock?

Also...  it looks like ppp_printing_proxy is not done. The calls to ppp_printing
need to be wrapped with CallWhileUnlocked.

That test must not have existed the last time I ran all these and fixed stuff
up, or something; I had all the ppapi_tests running a while back. It's also
likely that interfaces which don't have tests will have some similar problems.
But they're easy to fix once we find them.

[brettw, 2012-11-06 18:48:31]

Try server indicates you're right. I will look at this test more.

[brettw, 2012-11-06 19:20:41]

Okay, I'm pretty sure this is correct now. We weren't locking for resource
replies. PTAL

[dmichael (off chromium), 2012-11-06 19:24:10]

https://chromiumcodereview.appspot.com/11361075/diff/2006/ppapi/proxy/plugin_...
File ppapi/proxy/plugin_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/11361075/diff/2006/ppapi/proxy/plugin_...
ppapi/proxy/plugin_dispatcher.cc:203: ProxyAutoLock lock;
Shouldn't this lock cover it? Or can we get to OnMsgResourceReply some other
way?

[brettw, 2012-11-06 19:34:13]

There's two codepaths. I broke the "regular" one (locking twice) and fixed the
browser->plugin codepath. I'll actually get some better try results before
adding you again.

[dmichael (off chromium), 2012-11-08 04:22:51]

Do you still have that branch available to upload here? I was confused about how
you fixed it until I realized the CL doesn't have the committed change.

[brettw, 2012-11-08 05:30:15]

Whoops, I did so many try runs I'd gotten lost (turns out mostly just a lot of
flakiness). I can fix any suggestions you have in followups:
http://src.chromium.org/viewvc/chrome?view=rev&revision=166560

[dmichael (off chromium), 2012-11-08 16:04:08]

I glanced at it already in src.chromium.org, and it still lgtm.

I just prefer Rietveld to reflect reality; it can save time & trouble when
someone has to go digging through old CLs for something. I think you can
still upload to that issue if you still have the branch it came from.
Otherwise, oh well :)

On Wed, Nov 7, 2012 at 10:30 PM, <brettw@chromium.org> wrote:

> Whoops, I did so many try runs I'd gotten lost (turns out mostly just a
> lot of
> flakiness). I can fix any suggestions you have in followups:
>
http://src.chromium.org/**viewvc/chrome?view=rev&**revision=166560<http://src...
>
>
http://codereview.chromium.**org/11361075/<http://codereview.chromium.org/113...
>