PPAPI: Fix GetBrowserInterface race conditions

PPAPI: Fix GetBrowserInterface race conditions

- Previously, we had an unguarded bool flag for whether an interface lookup had been logged to UMA. Now each InterfaceInfo has a lock+flag. Should be near-zero contention.
- Previously, PpapiGlobals::GetBrowserSender did lazy creation with no synchronization. Now we create it at process startup and clear it at process shutdown, so there should be no race while the plugin is running.

BUG=413513, 414150
Committed: https://crrev.com/d1b2c8f719b0ab471a476bf53911a3657bb4c06a
Cr-Commit-Position: refs/heads/master@{#294715}

Committed: https://crrev.com/fee3a512a9456e0398ea8369496ac1c365fb3574
Cr-Commit-Position: refs/heads/master@{#295551}

[dmichael (off chromium), 2014-09-12 18:12:07]

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

[dmichael (off chromium), 2014-09-12 18:12:08]

WDYT?

https://codereview.chromium.org/568793002/diff/20001/ppapi/proxy/interface_li...
File ppapi/proxy/interface_list.cc (right):

https://codereview.chromium.org/568793002/diff/20001/ppapi/proxy/interface_li...
ppapi/proxy/interface_list.cc:355: new
PpapiHostMsg_LogInterfaceUsage(HashInterfaceName(name)));
note that previously, even with the "interface_logged" race fixed, we could
still enter GetBrowserSender from multiple threads if the interface name was
different. Hence why I fixed that race too.

https://codereview.chromium.org/568793002/diff/20001/ppapi/proxy/interface_li...
File ppapi/proxy/interface_list.h (right):

https://codereview.chromium.org/568793002/diff/20001/ppapi/proxy/interface_li...
ppapi/proxy/interface_list.h:78: NameToInterfaceInfoMap;
changed because AtomicSequenceNumber is not copyable

[teravest, 2014-09-12 19:08:28]

https://codereview.chromium.org/568793002/diff/20001/ppapi/proxy/interface_li...
File ppapi/proxy/interface_list.h (right):

https://codereview.chromium.org/568793002/diff/20001/ppapi/proxy/interface_li...
ppapi/proxy/interface_list.h:72: base::AtomicSequenceNumber
interface_request_count_;
Since we don't really need a sequence number, I wonder if it'd be better to use
either:
  CompareAndSwap, or
  an internal lock that's only held (briefly) when managing a flag.

Since this shouldn't be contended, now that I think about it, it seems like a
mutex+flag should be fine.

https://codereview.chromium.org/568793002/diff/20001/ppapi/proxy/plugin_globa...
File ppapi/proxy/plugin_globals.cc (right):

https://codereview.chromium.org/568793002/diff/20001/ppapi/proxy/plugin_globa...
ppapi/proxy/plugin_globals.cc:195: // CAUTION: This funciton is called without
the ProxyLock. See also
s/funciton/function

[dmichael (off chromium), 2014-09-12 20:44:00]

https://codereview.chromium.org/568793002/diff/20001/ppapi/proxy/interface_li...
File ppapi/proxy/interface_list.h (right):

https://codereview.chromium.org/568793002/diff/20001/ppapi/proxy/interface_li...
ppapi/proxy/interface_list.h:72: base::AtomicSequenceNumber
interface_request_count_;
done with a lock-per-interface. Contention should be incredibly rare.

https://codereview.chromium.org/568793002/diff/20001/ppapi/proxy/plugin_globa...
File ppapi/proxy/plugin_globals.cc (right):

https://codereview.chromium.org/568793002/diff/20001/ppapi/proxy/plugin_globa...
ppapi/proxy/plugin_globals.cc:195: // CAUTION: This funciton is called without
the ProxyLock. See also
On 2014/09/12 19:08:27, teravest wrote:
> s/funciton/function

Done.

[teravest, 2014-09-12 21:00:17]

lgtm

It would be good to put a description of how you fixed the race conditions in
the commit description.

https://codereview.chromium.org/568793002/diff/80001/ppapi/proxy/interface_li...
File ppapi/proxy/interface_list.cc (right):

https://codereview.chromium.org/568793002/diff/80001/ppapi/proxy/interface_li...
ppapi/proxy/interface_list.cc:370: if (!sent_to_uma_) {
nit: I believe you could reduce the critical section as follows:
{
  {
    base::AutoLock acquire(sent_to_uma_lock_);
    if (sent_to_uma_) {
      return;
    }
    sent_to_uma_ = true;
  }
  PluginGlobals::Get()->GetBrowserSender()->Send(...);
}

It's only sent_to_uma_ that has to be protected under the lock, not the
GetBrowserSender()/Send() calls.

...that said, what's here is probably acceptable too.

[dmichael (off chromium), 2014-09-12 22:21:19]

https://codereview.chromium.org/568793002/diff/80001/ppapi/proxy/interface_li...
File ppapi/proxy/interface_list.cc (right):

https://codereview.chromium.org/568793002/diff/80001/ppapi/proxy/interface_li...
ppapi/proxy/interface_list.cc:370: if (!sent_to_uma_) {
On 2014/09/12 21:00:17, teravest wrote:
> nit: I believe you could reduce the critical section as follows:
> {
>   {
>     base::AutoLock acquire(sent_to_uma_lock_);
>     if (sent_to_uma_) {
>       return;
>     }
>     sent_to_uma_ = true;
>   }
>   PluginGlobals::Get()->GetBrowserSender()->Send(...);
> }
> 
> It's only sent_to_uma_ that has to be protected under the lock, not the
> GetBrowserSender()/Send() calls.
> 
> ...that said, what's here is probably acceptable too.
Good point. Probably has no practical performance impact, but it makes it more
obvious what we're actually guarding. Done.

[dmichael (off chromium), 2014-09-12 22:21:23]

The CQ bit was checked by dmichael@chromium.org

[commit-bot: I haz the power, 2014-09-12 22:22:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/568793002/100001

[commit-bot: I haz the power, 2014-09-12 22:35:06]

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

[commit-bot: I haz the power, 2014-09-12 22:35:06]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dmichael (off chromium), 2014-09-12 22:38:11]

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

[dmichael (off chromium), 2014-09-12 22:38:12]

piman for owners of content/ppapi_plugin. Trivial name change.

[piman, 2014-09-12 23:48:14]

lgtm

[dmichael (off chromium), 2014-09-13 00:11:59]

The CQ bit was checked by dmichael@chromium.org

[commit-bot: I haz the power, 2014-09-13 00:18:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/568793002/100001

[commit-bot: I haz the power, 2014-09-13 01:09:33]

Committed patchset #6 (id:100001) as 8261a0a3324670b9cfa790ac122ffd1422ca0431

[commit-bot: I haz the power, 2014-09-13 01:17:42]

Patchset 6 (id:??) landed as
https://crrev.com/d1b2c8f719b0ab471a476bf53911a3657bb4c06a
Cr-Commit-Position: refs/heads/master@{#294715}

[raymes, 2014-09-15 03:45:07]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/566243004/ by raymes@chromium.org.

The reason for reverting is: Sorry to revert but I randomly noticed this to be
causing a top crasher e.g. go/crash/75090c1e31f33cfd

The reason is because the patch explicitly calls
PluginGlobals::SetPluginProxyDelegate with NULL from PpapiThread::Shutdown but
the pointer is used immediately inside the function.

I guess it should be a simple fix. I was going to put up a fix which just reset
the browser_sender_ to NULL in that case, but then I noticed there were cases
where we call GetBrowserSender() and use it without checking whether it is NULL
(e.g.
https://code.google.com/p/chromium/codesearch#search/&q=%22PluginGlobals::Get...)

I felt it would be better to just revert for now, sorry :(.

[dmichael (off chromium), 2014-09-15 16:34:31]

At shutdown, ppapi_thread.cc resets plugin_proxy_delegate_, which was leading to
a NULL pointer dereference within SetPluginProxyDelegate. I don't totally love
this patch, since ideally GetBrowserSender would be behind a lock. OTOH I think
it's better than the old way, where GetBrowserSender would directly dereference
a NULL pointer if it was called after plugin_proxy_delegate_ was reset. Unless
the plugin is failing to quit/join on a background thread, there's no race.
(Note this path doesn't happen at all for NaCl, which is simply killed at
shutdown. Probably primarily possible in Flash.)

WDYT?

[teravest, 2014-09-15 21:58:26]

On 2014/09/15 16:34:31, dmichael wrote:
> At shutdown, ppapi_thread.cc resets plugin_proxy_delegate_, which was leading
to
> a NULL pointer dereference within SetPluginProxyDelegate. I don't totally love
> this patch, since ideally GetBrowserSender would be behind a lock. OTOH I
think
> it's better than the old way, where GetBrowserSender would directly
dereference
> a NULL pointer if it was called after plugin_proxy_delegate_ was reset. Unless
> the plugin is failing to quit/join on a background thread, there's no race.
> (Note this path doesn't happen at all for NaCl, which is simply killed at
> shutdown. Probably primarily possible in Flash.)
> 
> WDYT?

lgtm, except for one style thing.

It seems like SetPluginProxyDelegate is really two separate functions now.

How about SetPluginProxyDelegate/ResetPluginProxyDelegate

[dmichael (off chromium), 2014-09-18 16:46:47]

The CQ bit was checked by dmichael@chromium.org

[commit-bot: I haz the power, 2014-09-18 16:47:54]

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

[commit-bot: I haz the power, 2014-09-18 21:33:23]

Committed patchset #8 (id:140001) as 60609f3328b69091e3b405391dd188568dcf00c5

[commit-bot: I haz the power, 2014-09-18 21:35:43]

Patchset 8 (id:??) landed as
https://crrev.com/fee3a512a9456e0398ea8369496ac1c365fb3574
Cr-Commit-Position: refs/heads/master@{#295551}