1433855 - Crash in mozilla::plugins::FunctionBroker<T>::PostToDispatchThread

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect, P1)

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is
report bp-fd382b98-2a80-4aec-89e4-abd060180129.
=============================================================

Top 10 frames of crashing thread:

0 ntdll.dll NtWaitForKeyedEvent 
1 ntdll.dll RtlSleepConditionVariableCS 
2 kernel32.dll SleepConditionVariableCS 
3 mozglue.dll mozilla::detail::ConditionVariableImpl::wait mozglue/misc/ConditionVariable_windows.cpp:58
4 xul.dll mozilla::CondVar::Wait xpcom/threads/CondVar.h:68
5 xul.dll mozilla::plugins::FunctionBroker<1, short >::PostToDispatchThread dom/plugins/ipc/FunctionBroker.h:1447
6 xul.dll mozilla::plugins::FunctionBroker<1, short >::BrokerCallClient dom/plugins/ipc/FunctionBroker.h:1340
7 xul.dll mozilla::plugins::FunctionBroker<1, short >::MaybeBrokerCallClient dom/plugins/ipc/FunctionBroker.h:1313
8 xul.dll mozilla::plugins::FunctionBroker<1, short >::InterceptorStub dom/plugins/ipc/FunctionBroker.h:1231
9 npswf64_28_0_0_137.dll F905260817____________________________ F_578802783_______________________________________________________________:6145

=============================================================

There are 14 crashes (from are 8 installations) in nightly 60 with buildid 20180128220152. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1382251.

[1] https://hg.mozilla.org/mozilla-central/rev?node=1e37c536895f

Comment 1

[Calixte Denizet (:calixte)]

Most of these signatures are top-crashers in nightly for plugin process.

Comment 2

[David Parks [:handyman]]

Looking into this.  Unfortunately, I don't have any leads yet.

Comment 3

[David Parks [:handyman]]

FYI, the crashes in some of the signatures are not tied to bug 1382251 (they may be related -- I'm still looking).  For example, [@nsThread::Dispatch] [1] has crashes going back to 1/24 but bug 1382251 (which is definitely responsible for some of of the signatures) went in on 1/27.

[1] https://crash-stats.mozilla.com/signature/?signature=nsThread%3A%3ADispatch

Comment 4

[David Parks [:handyman]]

Attachment: Make sure plugin function broker's PostToDispatchThread is Waiting before Notifying it

This patch attempts to correct a synchronization issue in the PostToDispatchThread, but (I think) it butts up against an inconsistency in the docs for the synchronization primitives so it may be wrong.  I also don't have an STR or anything like that and I can't say that I see how this would cause the crash.  But it is a bug that could cause some failure.  Note that some of the crash signatures say that this is a hang, which is what I'd expect if this is the cause for the bug.

Here's the location of the failure [1]:

>  void FunctionBroker::PostToDispatchThread {
>  [...]
>  // Run PostToDispatchHelper on the dispatch thread.  It will notify our
>  // waiting monitor when it is done.
>  Monitor monitor("FunctionDispatchThread Lock");
>  MonitorAutoLock lock(monitor);
>  bool success = false;
>  FunctionBrokerChild::GetInstance()->PostToDispatchThread(
>    NewRunnableFunction("FunctionDispatchThreadRunnable", &PostToDispatchHelper,
>                        this, &monitor, &success, &aWinError, &aRet,
>                        &aParameters...));
>  monitor.Wait();
>  return success;
>  }

And the runnable it wants to dispatch [2]:

>  static void
>  PostToDispatchHelper(const SelfType* bmhi, Monitor* monitor, bool* ok, uint32_t* winErr, ResultType* r, ParamTypes*... p)
>  {
>    // Note: p is also non-null... its just hard to assert that.
>    MOZ_ASSERT(bmhi && monitor && ok && winErr && r);
>    *ok = bmhi->BrokerCallClient(*winErr, *r, *p...);
>    *ok &= NS_SUCCEEDED(monitor->Notify());
>  };

The issue would come in the unlikely event that the PostToDispatchThread happened, the dispatch thread ran PostToDispatchHelper, got its response and notified the monitor -- all before PostToDispatchThread got to the next line, monitor.Wait().  Obviously, the Wait() misses the Notify() in that case and you get a hang.  Now, the way to fix this is to Lock() the monitor in PostToDispatchHelper before calling Notify().  (Remember, the first thing Wait() does is release the monitor so this is ok.)  Note that we dont need the lock for the BrokerCallClient call -- there is no data race.  This is all about the timing of the Notify call.

So where does PostToDispatchHelper free the lock?  This is where the docs come in.  In general, Monitor is based on PRCondVar and PR_NotifyCondVar says [3]:

> ** Returns PR_FAILURE if the caller has not locked the lock associated
> ** with the condition variable.

...meaning we should (err, must) hold the lock during the Notify call.  This is obviously not true since we don't see PR_FAILURE -- it just works.  The deal is that the Windows implementation of Monitor uses a non-POSIX backend [4]:

> WakeConditionVariable(&platformData()->cv_);

...and, with CONDITION_VARIABLEs (i.e. cv_ above), we _dont_ need the lock before, as seen in this MSDN example [5].  I dont know if its an error to hold the lock before notifying on Windows but I know that we can free the lock _before_ notifying and still get the sequential guarantee we need to fix _this_ bug.  So thats what I do.  I expect this wouldn't work on POSIX-based platforms as-is but it will tell us for sure if this mistake is related to this bug.

(To be clear, if this works and Windows allows Notify on a Locked monitor, then we can just move the Notify call into the block with the MonitorAutoLock to fix POSIX.  Im currently just trying to avoid trading one error for another, which would mean I'd learn nothing from this patch in that case.)

Trying tests now [6]

---------

[1] https://searchfox.org/mozilla-central/rev/c56f656febb1aa58c72b218699e24a76425bd284/dom/plugins/ipc/FunctionBroker.h#1440
[2] https://searchfox.org/mozilla-central/rev/c56f656febb1aa58c72b218699e24a76425bd284/dom/plugins/ipc/FunctionBroker.h#1267
[3] https://searchfox.org/mozilla-central/rev/c56f656febb1aa58c72b218699e24a76425bd284/nsprpub/pr/include/prcvar.h#77
[4] https://searchfox.org/mozilla-central/rev/c56f656febb1aa58c72b218699e24a76425bd284/mozglue/misc/ConditionVariable_windows.cpp#45
[5] https://msdn.microsoft.com/en-us/ms686903(v=vs.85)
[6] https://treeherder.mozilla.org/#/jobs?repo=try&revision=08a25dcf060515054e64a71b6c5615532097f682

Comment 5

[David Parks [:handyman]]

Comment on attachment 8946958 [details] [diff] [review]
Make sure plugin function broker's PostToDispatchThread is Waiting before Notifying it

Still no STR.  See my last comment.

Comment 6

[Jim Mathies [:jimm]]

Comment on attachment 8946958 [details] [diff] [review]
Make sure plugin function broker's PostToDispatchThread is Waiting before Notifying it

thanks for the detailed explanation, lets land it and see.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cc1a1751a0e
Make sure plugin function broker's PostToDispatchThread is Waiting before Notifying it. r=jimm

Comment 8

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/5cc1a1751a0e

Comment 10

[Marcia Knous [:marcia]]

I am still seeing a number of plugin crashes in the latest nightly: http://bit.ly/2EgKI63. Not all the signatures, but specifically some of the flash ones (top 7 in the crash signatures list).

Comment 11

[Marcia Knous [:marcia]]

(In reply to Marcia Knous [:marcia - use ni] from comment #10)
> I am still seeing a number of plugin crashes in the latest nightly:
> http://bit.ly/2EgKI63. Not all the signatures, but specifically some of the
> flash ones (top 7 in the crash signatures list).

Should I reopen this bug? Or do we want the continuing issue in some signatures in a new bug?

Comment 12

[David Parks [:handyman]]

I hadn't gone over the whole list closely enough but I believe there are at least two different bugs here.  The signatures that have tapered off to almost (!) zero are related to the Monitor fix we added but the others, the ones with only Flash+garbage stacks, are something else.  Their timing suggests their cause is the same original patch but they make me think there is a function Flash is calling that I'm returning junk for -- and messing it up.

So its probably a good idea to create a new bug for the crashes that didn't almost go away.  The others (the ones with FunctionBroker in the signature) need to be addressed eventually but aren't a priority.  Big thanks, Marcia.

Comment 13

[David Parks [:handyman]]

Marcia,

Unless you've already started making a new one, I think you can just re-open this bug.  I've spent a few more hours cross-checking and there is definitely a correlation between the "mystery stacks" full of garbage and the ones with FunctionBroker in them.  I'm not 100% certain they are the same thing but the odds now feel more strongly than not that they are.  Apologies for not catching that sooner.

Comment 14

[David Parks [:handyman]]

...and we come full circle.

Marcia (I'm deleting the NI), I think I've found what could be a cause for this.  Its in about the same spot but its not really related to this patch (it fixes something else).  So its easier to leave this as-is and add the new fix in a new bug, which I'm doing now.

Comment 15

[Marcia Knous [:marcia]]

(In reply to David Parks (dparks) [:handyman] from comment #14)
> ...and we come full circle.
> 
> Marcia (I'm deleting the NI), I think I've found what could be a cause for
> this.  Its in about the same spot but its not really related to this patch
> (it fixes something else).  So its easier to leave this as-is and add the
> new fix in a new bug, which I'm doing now.

Thanks David - I assume the new bug is Bug 1436253? Also the crash rate is very high on nightly because of these flash and plugin crashes.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Sorry, I misread comment 14.

Comment 17

[David Parks [:handyman]]

Yep, bug 1436253.  Good news: its already got a patch that I have high hopes for.  Bad news: I would probably have said that here, too.