Closed
Bug 1343639
Opened 8 years ago
Closed 8 years ago
Get rid of HangObserverNotifier in PHM in favour of a task on the mMainThreadTaskFactory
Categories
(Core :: DOM: Content Processes, enhancement)
Core
DOM: Content Processes
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 3•8 years ago
|
||
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 5•8 years ago
|
||
mozreview-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
::: 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+
Reporter | ||
Comment 6•8 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 9•8 years ago
|
||
mozreview-review |
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+
Comment 10•8 years ago
|
||
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
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/469ca7b864f2
https://hg.mozilla.org/mozilla-central/rev/0bffb87589b8
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•