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

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, enhancement)

Description

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1343639 - Allow TaskFactory::NewRunnableMethod pass an arbitrary number of arguments to the runnable.

Review commit: https://reviewboard.mozilla.org/r/117686/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/117686/

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1343639 - Use the HangMonitorParent TaskFactory instead of runnables for dispatching the hang notification to the main thread.

Review commit: https://reviewboard.mozilla.org/r/117688/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/117688/

Comment 3

[Mike Conley (:mconley) (:⚙️)]

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).

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

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 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8844149 [details]
Bug 1343639 - Allow TaskFactory::NewRunnableMethod pass an arbitrary number of arguments to the runnable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117686/diff/1-2/

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8844150 [details]
Bug 1343639 - Use the HangMonitorParent TaskFactory instead of runnables for dispatching the hang notification to the main thread.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117688/diff/1-2/

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/469ca7b864f2
https://hg.mozilla.org/mozilla-central/rev/0bffb87589b8