Hold the Pepper proxy lock for all Pepper GL calls.

Hold the Pepper proxy lock for all Pepper GL calls.

BUG=418651
Committed: https://crrev.com/057f7a69dd40c93550b375b4287dba70d719316a
Cr-Commit-Position: refs/heads/master@{#302303}

Committed: https://crrev.com/5a9a39b79e40a322b7f6c2d8051f99a49f2fe598
Cr-Commit-Position: refs/heads/master@{#303658}

[Peng, 2014-10-30 20:39:02]

penghuang@chromium.org changed reviewers:
+ dmichael@chromium.org

[Peng, 2014-10-30 20:39:02]

Hi David, PTAL. Thanks.

[dmichael (off chromium), 2014-10-30 21:05:10]

Assuming you've verified that this fixes your problem, lgtm

[dmichael (off chromium), 2014-10-30 21:20:56]

Noticed a couple of things, o/w still lgtm

https://codereview.chromium.org/692683003/diff/1/ppapi/proxy/plugin_dispatcher.h
File ppapi/proxy/plugin_dispatcher.h (right):

https://codereview.chromium.org/692683003/diff/1/ppapi/proxy/plugin_dispatche...
ppapi/proxy/plugin_dispatcher.h:144: virtual bool Send(IPC::Message* msg);
Wait, why are these marked virtual? I would expect "override".

https://codereview.chromium.org/692683003/diff/1/ppapi/proxy/plugin_dispatche...
ppapi/proxy/plugin_dispatcher.h:145: virtual bool
SendAndStayLocked(IPC::Message* msg);
And this one is not an override and should not be virtual.

Also, please add a comment indicating that while Send will release the proxy
lock on outgoing synchronous messages, SendAndStayLocked continues to hold the
lock unconditionally.

[Peng, 2014-10-31 13:50:56]

https://codereview.chromium.org/692683003/diff/1/ppapi/proxy/plugin_dispatcher.h
File ppapi/proxy/plugin_dispatcher.h (right):

https://codereview.chromium.org/692683003/diff/1/ppapi/proxy/plugin_dispatche...
ppapi/proxy/plugin_dispatcher.h:144: virtual bool Send(IPC::Message* msg);
On 2014/10/30 21:20:56, dmichael wrote:
> Wait, why are these marked virtual? I would expect "override".

Done.

https://codereview.chromium.org/692683003/diff/1/ppapi/proxy/plugin_dispatche...
ppapi/proxy/plugin_dispatcher.h:145: virtual bool
SendAndStayLocked(IPC::Message* msg);
On 2014/10/30 21:20:56, dmichael wrote:
> And this one is not an override and should not be virtual.
> 
> Also, please add a comment indicating that while Send will release the proxy
> lock on outgoing synchronous messages, SendAndStayLocked continues to hold the
> lock unconditionally.

Actually this one is a virtual function, because the CommandProxy hold a
ProxyChannel instead of this class.

Comment added. Done

[dmichael(do not use this one), 2014-10-31 14:49:31]

dmichael@google.com changed reviewers:
+ dmichael@google.com

[dmichael(do not use this one), 2014-10-31 14:49:32]

lgtm

https://codereview.chromium.org/692683003/diff/20001/ppapi/proxy/plugin_dispa...
File ppapi/proxy/plugin_dispatcher.h (right):

https://codereview.chromium.org/692683003/diff/20001/ppapi/proxy/plugin_dispa...
ppapi/proxy/plugin_dispatcher.h:151: virtual void OnChannelError() override;
The new guidance is to use only one of "virtual" or "override". So please take
off the "virtual" part for all of these overrides. (Sorry, I know a lot of this
was pre-existing)

[Peng, 2014-10-31 14:55:38]

https://codereview.chromium.org/692683003/diff/20001/ppapi/proxy/plugin_dispa...
File ppapi/proxy/plugin_dispatcher.h (right):

https://codereview.chromium.org/692683003/diff/20001/ppapi/proxy/plugin_dispa...
ppapi/proxy/plugin_dispatcher.h:151: virtual void OnChannelError() override;
On 2014/10/31 14:49:32, dmichael(do not use this one) wrote:
> The new guidance is to use only one of "virtual" or "override". So please take
> off the "virtual" part for all of these overrides. (Sorry, I know a lot of
this
> was pre-existing)
I see. Done

[Peng, 2014-10-31 16:19:54]

The CQ bit was checked by penghuang@chromium.org

[commit-bot: I haz the power, 2014-10-31 16:20:07]

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

[commit-bot: I haz the power, 2014-10-31 19:02:41]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-10-31 19:03:18]

Patchset 3 (id:??) landed as
https://crrev.com/057f7a69dd40c93550b375b4287dba70d719316a
Cr-Commit-Position: refs/heads/master@{#302303}

[kareng, 2014-11-01 19:05:38]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/698463006/ by kareng@google.com.

The reason for reverting is: causing huge crashes in canary. bug=429446.

[dmichael (off chromium), 2014-11-11 15:48:05]

dmichael@chromium.org changed reviewers:
- dmichael@google.com

[dmichael (off chromium), 2014-11-11 15:49:01]

The underlying cause of the crash this introduced has been fixed in:
https://chromium.googlesource.com/chromium/src/+/a17de8faa97ca33d1e58e08392db...

So I think this is ready to re-land.

[dmichael (off chromium), 2014-11-11 15:49:07]

The CQ bit was checked by dmichael@chromium.org

[commit-bot: I haz the power, 2014-11-11 15:49:37]

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

[Peng, 2014-11-11 15:50:26]

Cool. Thanks.

On Tue Nov 11 2014 at 10:49:04 AM <dmichael@chromium.org> wrote:

> The underlying cause of the crash this introduced has been fixed in:
> https://chromium.googlesource.com/chromium/src/+/
> a17de8faa97ca33d1e58e08392dbdc3e316d6cbe
>
> So I think this is ready to re-land.
>
>
> https://codereview.chromium.org/692683003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-11-11 16:32:23]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-11-11 16:33:02]

Patchset 3 (id:??) landed as
https://crrev.com/5a9a39b79e40a322b7f6c2d8051f99a49f2fe598
Cr-Commit-Position: refs/heads/master@{#303658}