Closed Bug 1343639 Opened 3 years ago Closed 3 years ago

Get rid of HangObserverNotifier in PHM in favour of a task on the mMainThreadTaskFactory

Categories

(Core :: DOM: Content Processes, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: mconley, Unassigned)

References

Details

Attachments

(2 files)

Bug 1342927 added a TaskFactory to HangMonitorParent which can be used to safely dispatch tasks between the main and monitor threads while also ensuring that those tasks won't be serviced if the HangMonitorParent ever goes away.

HangObserverNotifier is an nsIRunnable that is dispatched from the monitor thread to the main thread in HangMonitorParent::RecvHangEvidence. There's the potential for an event on the main thread to cause the HangMonitorParent to be destroyed before the nsIRunnable is fired, which would be bad, since the runnable attempts to access the HangMonitorParent, which would cause us to crash.

Using the TaskFactory makes it so that if the HangMonitorParent is destroyed, and queued runnables from another thread are just cancelled.
Hey billm,

I've hit a slight snag here. I followed the examples I found elsewhere in the tree to add the rest arguments to NewRunnableMethod, and that included the && C++ Move stuff that I'm reallllly not too familiar with just yet.

At any rate, the snag seems to be that a Move constructor isn't defined for the HangData structure, which is set up in IPDL.

Is my assumption wrong that we have to use Move for the rest args? Or is there a way I can add Move to HangData via its IPDL definition? (searchfox couldn't find an example of this).
Flags: needinfo?(wmccloskey)
You should be able to pass things to things that don't have a move constructor to NewRunnableMethod. It uses a technique called "perfect forwarding". It's one of those useful C++ things that's very difficult to explain:
http://thbecker.net/articles/rvalue_references/section_07.html

That said, it can be really tricky to get this stuff right. Are you getting compile errors? The code looks okay to me.
Flags: needinfo?(wmccloskey)
Comment on attachment 8844149 [details]
Bug 1343639 - Allow TaskFactory::NewRunnableMethod pass an arbitrary number of arguments to the runnable.

https://reviewboard.mozilla.org/r/117686/#review119790

::: ipc/glue/TaskFactory.h:61
(Diff revision 1)
>    template <class Method>
>    inline already_AddRefed<Runnable> NewRunnableMethod(Method method) {
>      typedef TaskWrapper<RunnableMethod<Method, Tuple0> > TaskWrapper;
>  
>      RefPtr<TaskWrapper> task = new TaskWrapper(this, object_, method,
>                                                 base::MakeTuple());
>  
>      return task.forget();
>    }
>  
>    template <class Method, class A>
>    inline already_AddRefed<Runnable> NewRunnableMethod(Method method, const A& a) {
>      typedef TaskWrapper<RunnableMethod<Method, Tuple1<A> > > TaskWrapper;
>  
>      RefPtr<TaskWrapper> task = new TaskWrapper(this, object_, method,
>                                                 base::MakeTuple(a));
>  
>      return task.forget();
>    }

These methods should be removed.

::: ipc/glue/TaskFactory.h:83
(Diff revision 1)
>      return task.forget();
>    }
>  
> +  template <class Method, typename... Args>
> +  inline already_AddRefed<Runnable>
> +  NewRunnableMethod(Method method, const Args&&... args) {

Why are the Args declared const? That part seems wrong to me. An actual rvalue reference will not be const, so it won't match. On the other hand, if you want to pass const, that part will be inferred as part of the Args type if you don't include it.

r+ if the patch works without const.

::: ipc/glue/TaskFactory.h:85
(Diff revision 1)
>  
> +  template <class Method, typename... Args>
> +  inline already_AddRefed<Runnable>
> +  NewRunnableMethod(Method method, const Args&&... args) {
> +    typedef TaskWrapper<
> +      RunnableMethod<Method, mozilla::Tuple<typename mozilla::Decay<Args>::Type...>>

Using multiple typedefs might be more readable here.
Attachment #8844149 - Flags: review?(wmccloskey) → review+
Comment on attachment 8844149 [details]
Bug 1343639 - Allow TaskFactory::NewRunnableMethod pass an arbitrary number of arguments to the runnable.

https://reviewboard.mozilla.org/r/117686/#review119790

> Why are the Args declared const? That part seems wrong to me. An actual rvalue reference will not be const, so it won't match. On the other hand, if you want to pass const, that part will be inferred as part of the Args type if you don't include it.
> 
> r+ if the patch works without const.

Yeah, good call. botond explained to me why `const` doesn't make sense here. He also helped me through using `decltype` to make sure I construct the proper Tuple from tuple.h.
Comment on attachment 8844150 [details]
Bug 1343639 - Use the HangMonitorParent TaskFactory instead of runnables for dispatching the hang notification to the main thread.

https://reviewboard.mozilla.org/r/117688/#review121116
Attachment #8844150 - Flags: review?(wmccloskey) → review+
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/469ca7b864f2
Allow TaskFactory::NewRunnableMethod pass an arbitrary number of arguments to the runnable. r=billm
https://hg.mozilla.org/integration/autoland/rev/0bffb87589b8
Use the HangMonitorParent TaskFactory instead of runnables for dispatching the hang notification to the main thread. r=billm
https://hg.mozilla.org/mozilla-central/rev/469ca7b864f2
https://hg.mozilla.org/mozilla-central/rev/0bffb87589b8
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.