If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Consolidate on one set of runnable method/function macros

RESOLVED FIXED in Firefox 49

Status

()

Core
IPC
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(8 attachments)

14.20 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
28.15 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
17.49 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
11.19 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
3.90 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
74.42 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
479.57 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
20.29 KB, patch
billm
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
Created attachment 8746327 [details] [diff] [review]
Part 1: Be explicit about which NewRunnableMethod callers want CancelableRunnables
Attachment #8746327 - Flags: review?(nfroyd)
Created attachment 8746328 [details] [diff] [review]
Part 2: Replace some NewRunnableMethods with NS_NewNonOwningRunnableMethod

Some callers of NewRunnableMethod have overridden their RunnableMethodTraits to do nothing.  These are easy to convert.

NB: A bunch of the RefPtr<T> runnable stuff will go away again later.
Created attachment 8746329 [details] [diff] [review]
Part 3: Replace some NewRunnableMethods with NS_NewNonOwningCancelableRunnableMethod

Like part 2 but for cancelable things.  After this there are no uses of RunnableMethodTraits left.
Created attachment 8746330 [details] [diff] [review]
Part 4: Replace NewCancelableRunnableMethod
Attachment #8746330 - Flags: review?(nfroyd)
Created attachment 8746331 [details] [diff] [review]
Part 5: Make NS_NewRunnableMethod able to call const functions
Attachment #8746331 - Flags: review?(nfroyd)
Created attachment 8746332 [details] [diff] [review]
Part 6: Replace NewRunnableMethod
Attachment #8746332 - Flags: review?(nfroyd)
Created attachment 8746333 [details] [diff] [review]
Part 7: Move NS_NewRunnableMethod* to mozilla::NewRunnableMethod

This textually undoes a lot of the previous patches, but it was useful to verify I was doing things correctly along the way.
Attachment #8746333 - Flags: review?(nfroyd)
Attachment #8746328 - Flags: review?(nfroyd)
Attachment #8746329 - Flags: review?(nfroyd)
Comment on attachment 8746327 [details] [diff] [review]
Part 1: Be explicit about which NewRunnableMethod callers want CancelableRunnables

Review of attachment 8746327 [details] [diff] [review]:
-----------------------------------------------------------------

Explicit is better than implicit.
Attachment #8746327 - Flags: review?(nfroyd) → review+
Comment on attachment 8746328 [details] [diff] [review]
Part 2: Replace some NewRunnableMethods with NS_NewNonOwningRunnableMethod

Review of attachment 8746328 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/ProcessHangMonitor.cpp
@@ +380,5 @@
> +                                          unsigned>(this,
> +                                                    &HangMonitorChild::NotifySlowScriptAsync,
> +                                                    id, filename, aLineNo);
> +  MonitorLoop()->PostTask(runnable.forget());
> +                      

Nit: whitespace.
Attachment #8746328 - Flags: review?(nfroyd) → review+
Comment on attachment 8746329 [details] [diff] [review]
Part 3: Replace some NewRunnableMethods with NS_NewNonOwningCancelableRunnableMethod

Review of attachment 8746329 [details] [diff] [review]:
-----------------------------------------------------------------

The patch summary should really read "replace some NewCancelableRunnableMethods with NS_NewNonOwningCancelableRunnableMethod", right?

::: gfx/layers/AtomicRefCountedWithFinalize.h
@@ -68,5 @@
>      template<class U>
>      friend struct mozilla::RefPtrTraits;
>  
> -    template<class U>
> -    friend struct ::RunnableMethodTraits;

\o/

::: gfx/layers/ipc/ImageBridgeParent.cpp
@@ +100,5 @@
>  {
>    // Can't alloc/dealloc shmems from now on.
>    mClosed = true;
>  
> +    if (mSubprocess) {

Nit: indentation went wonky here.

@@ +106,5 @@
>      mSubprocess = nullptr;
>    }
>  
> +  RefPtr<Runnable> runnable = NS_NewRunnableMethod(this, &ImageBridgeParent::DeferredDestroy);
> +  MessageLoop::current()->PostTask(runnable.forget());

Does this change go someplace else?

::: ipc/glue/MessageChannel.cpp
@@ +497,5 @@
>  
> +    RefPtr<CancelableRunnable> runnable =
> +        NS_NewNonOwningCancelableRunnableMethod(this, &MessageChannel::OnMaybeDequeueOne);
> +    mDequeueOneTask = new RefCountedTask(runnable.forget());
> +                                                 

Nit: whitespace.

::: xpcom/glue/nsThreadUtils.h
@@ +286,5 @@
> +         bool Owning = true,
> +         bool Cancelable = false>
> +class nsRunnableMethod : public mozilla::Conditional<!Cancelable,
> +                                                     mozilla::Runnable,
> +                                                     mozilla::CancelableRunnable>::Type

Nice.

@@ +716,5 @@
>      }
>      return NS_OK;
>    }
> +  nsresult Cancel() {
> +    static_assert(Cancelable, "Don't use me!");

This works even when Cancelable is false?  Someday I will understand all the template instantiation rules...
Attachment #8746329 - Flags: review?(nfroyd) → review+
Attachment #8746330 - Flags: review?(nfroyd) → review+
Comment on attachment 8746331 [details] [diff] [review]
Part 5: Make NS_NewRunnableMethod able to call const functions

Review of attachment 8746331 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/glue/nsThreadUtils.h
@@ +342,5 @@
>    static const bool can_cancel = Cancelable;
>  };
>  
> +template<class C, typename R, bool Owning, bool Cancelable, typename... As>
> +struct nsRunnableMethodTraits<R(C::*)(As...) const, Owning, Cancelable>

Probably worth duplicating this for the NS_HAVE_STDCALL cases below just so somebody doesn't wind up pounding their head against template-fu later on.
Attachment #8746331 - Flags: review?(nfroyd) → review+
Comment on attachment 8746332 [details] [diff] [review]
Part 6: Replace NewRunnableMethod

Review of attachment 8746332 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with changes below and pending adequate explanations to the questions.

::: dom/ipc/ContentBridgeChild.cpp
@@ +34,5 @@
>  void
>  ContentBridgeChild::ActorDestroy(ActorDestroyReason aWhy)
>  {
> +  RefPtr<Runnable> runnable = NS_NewRunnableMethod(this, &ContentBridgeChild::DeferredDestroy);
> +  MessageLoop::current()->PostTask(runnable.forget());

I'm starting to think NS_New*RunnableMethod should just return an already_AddRefed, so we can make this sort of stuff shorter.  Dollars to donuts somebody is going to complain about this when you announce the changes, if they haven't already.  This can be a followup.

::: dom/ipc/ContentBridgeParent.h
@@ +81,5 @@
>  
> +  void Close()
> +  {
> +    // Trick NS_NewRunnableMethod
> +    PContentBridgeParent::Close();

Uh.  This is weird.  Can you elaborate?  Why not cast to PContentBridgeParent when calling NS_NewRunnableMethod?

::: dom/ipc/TabChild.cpp
@@ +604,5 @@
>    mAsyncPanZoomEnabled = gfxPlatform::AsyncPanZoomEnabled();
>  
>    nsWeakPtr weakPtrThis(do_GetWeakReference(static_cast<nsITabChild*>(this)));  // for capture by the lambda
>    mSetAllowedTouchBehaviorCallback = [weakPtrThis](uint64_t aInputBlockId,
> +                                                   nsTArray<TouchBehaviorFlags>&& aFlags)

So we have a couple of these sorts of changes...can you explain why they're necessary and safe?  Some of them look pretty dodgy.

::: dom/media/gmp/GMPDecryptorChild.cpp
@@ +60,5 @@
>    } else {
>      // Use const reference when we have to.
>      auto m = &GMPDecryptorChild::CallMethod<
>          decltype(aMethod), typename AddConstReference<ParamType>::Type...>;
> +    RefPtr<mozilla::Runnable> t = shitty_code::NewRunnableMethod(this, m, aMethod, Forward<ParamType>(aParams)...);

Why is this?  Is NS_NewRunnableMethodWithArgs's template-fu not strong enough here?

::: ipc/chromium/src/base/task.h
@@ +304,5 @@
>  };
>  
> +namespace shitty_code {
> +
> +// Don't add new uses of this!!!!

I appreciate the tactic here; the name is a tad unprofessional.

How about |namespace dont_use_this_or_khuey_will_hunt_you_down|?  Is that any less unprofessional?  Maybe |namespace dont_use_this_without_khuey_reviewing|?

::: widget/nsBaseWidget.cpp
@@ +981,5 @@
> +                                   StoreCopyPassByRRef<nsTArray<TouchBehaviorFlags>>>(treeManager,
> +                                                                                      &APZCTreeManager::SetAllowedTouchBehavior,
> +                                                                                      aInputBlockId, mozilla::Move(aFlags));
> +    APZThreadUtils::RunOnControllerThread(runnable.forget());
> +                                                            

Nit: whitespace.
Attachment #8746332 - Flags: review?(nfroyd) → review+
Comment on attachment 8746333 [details] [diff] [review]
Part 7: Move NS_NewRunnableMethod* to mozilla::NewRunnableMethod

Review of attachment 8746333 [details] [diff] [review]:
-----------------------------------------------------------------

I appreciate this change being broken out into a separate patch.

::: xpcom/glue/nsThreadUtils.h
@@ +749,2 @@
>  template<typename PtrType, typename Method>
> +already_AddRefed<typename nsRunnableMethodTraits<Method, true, false>::base_type>

Bonus points for making this change.

@@ +883,5 @@
>      }
>      return *this;
>    }
>  
> +  const nsRevocableEventPtr& operator=(already_AddRefed<T> aEvent)

Why is this necessary amidst the sea of renaming?
Attachment #8746333 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #13)
> @@ +883,5 @@
> >      }
> >      return *this;
> >    }
> >  
> > +  const nsRevocableEventPtr& operator=(already_AddRefed<T> aEvent)
> 
> Why is this necessary amidst the sea of renaming?

Actually, never mind, I think I understand why this is, given the changes to NS_New*Runnable*.
Whiteboard: btpp-active
(In reply to Nathan Froyd [:froydnj] from comment #10)
> The patch summary should really read "replace some
> NewCancelableRunnableMethods with NS_NewNonOwningCancelableRunnableMethod",
> right?

Yes.

> @@ +106,5 @@
> >      mSubprocess = nullptr;
> >    }
> >  
> > +  RefPtr<Runnable> runnable = NS_NewRunnableMethod(this, &ImageBridgeParent::DeferredDestroy);
> > +  MessageLoop::current()->PostTask(runnable.forget());
> 
> Does this change go someplace else?

No ...

> @@ +716,5 @@
> >      }
> >      return NS_OK;
> >    }
> > +  nsresult Cancel() {
> > +    static_assert(Cancelable, "Don't use me!");
> 
> This works even when Cancelable is false?  Someday I will understand all the
> template instantiation rules...

Yes.  I believe it's because if Cancelable is false this doesn't override anything which means it's a regular member function on a template class, and only gets instantiated if it is used by something.  Whereas if it were overriding a virtual function on the base class then it would have to be instantiated.
(In reply to Nathan Froyd [:froydnj] from comment #12)
> Comment on attachment 8746332 [details] [diff] [review]
> Part 6: Replace NewRunnableMethod
> 
> Review of attachment 8746332 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with changes below and pending adequate explanations to the questions.
> 
> ::: dom/ipc/ContentBridgeChild.cpp
> @@ +34,5 @@
> >  void
> >  ContentBridgeChild::ActorDestroy(ActorDestroyReason aWhy)
> >  {
> > +  RefPtr<Runnable> runnable = NS_NewRunnableMethod(this, &ContentBridgeChild::DeferredDestroy);
> > +  MessageLoop::current()->PostTask(runnable.forget());
> 
> I'm starting to think NS_New*RunnableMethod should just return an
> already_AddRefed, so we can make this sort of stuff shorter.  Dollars to
> donuts somebody is going to complain about this when you announce the
> changes, if they haven't already.  This can be a followup.

:)

> ::: dom/ipc/ContentBridgeParent.h
> @@ +81,5 @@
> >  
> > +  void Close()
> > +  {
> > +    // Trick NS_NewRunnableMethod
> > +    PContentBridgeParent::Close();
> 
> Uh.  This is weird.  Can you elaborate?  Why not cast to
> PContentBridgeParent when calling NS_NewRunnableMethod?

NS_NewRunnableMethod(instance, &class::function); binds to the type of class, not the type of instance.  So here we do NS_NewRunnableMethod(contentBridgeParent, &PContentBridgeParent::Close), and it fails to compile because PContentBridgeParent is not a refcounted type.

> ::: dom/ipc/TabChild.cpp
> @@ +604,5 @@
> >    mAsyncPanZoomEnabled = gfxPlatform::AsyncPanZoomEnabled();
> >  
> >    nsWeakPtr weakPtrThis(do_GetWeakReference(static_cast<nsITabChild*>(this)));  // for capture by the lambda
> >    mSetAllowedTouchBehaviorCallback = [weakPtrThis](uint64_t aInputBlockId,
> > +                                                   nsTArray<TouchBehaviorFlags>&& aFlags)
> 
> So we have a couple of these sorts of changes...can you explain why they're
> necessary and safe?  Some of them look pretty dodgy.

Basically this array was being copied at every point and the chromium templates were hiding that.  I probably should have broken this out separately ...

> ::: dom/media/gmp/GMPDecryptorChild.cpp
> @@ +60,5 @@
> >    } else {
> >      // Use const reference when we have to.
> >      auto m = &GMPDecryptorChild::CallMethod<
> >          decltype(aMethod), typename AddConstReference<ParamType>::Type...>;
> > +    RefPtr<mozilla::Runnable> t = shitty_code::NewRunnableMethod(this, m, aMethod, Forward<ParamType>(aParams)...);
> 
> Why is this?  Is NS_NewRunnableMethodWithArgs's template-fu not strong
> enough here?

Right. NS_NewRunnableMethodWithArg requires you to specify the storage types, whereas the chromium template guesses.  The macro will have to be unwound manually to get rid of this.

> ::: ipc/chromium/src/base/task.h
> @@ +304,5 @@
> >  };
> >  
> > +namespace shitty_code {
> > +
> > +// Don't add new uses of this!!!!
> 
> I appreciate the tactic here; the name is a tad unprofessional.
> 
> How about |namespace dont_use_this_or_khuey_will_hunt_you_down|?  Is that
> any less unprofessional?  Maybe |namespace
> dont_use_this_without_khuey_reviewing|?

Fixed.
(In reply to Nathan Froyd [:froydnj] from comment #13)
> @@ +883,5 @@
> >      }
> >      return *this;
> >    }
> >  
> > +  const nsRevocableEventPtr& operator=(already_AddRefed<T> aEvent)
> 
> Why is this necessary amidst the sea of renaming?

Because I changed the method-previously-known-as-NS_NewRunnableMethod to return already_AddRefed, and nsRevocableEventPtr doesn't accept already_AddRefed yet.  This has nothing to do with the renaming.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #16)
> (In reply to Nathan Froyd [:froydnj] from comment #12)
> > ::: dom/ipc/ContentBridgeChild.cpp
> > @@ +34,5 @@
> > >  void
> > >  ContentBridgeChild::ActorDestroy(ActorDestroyReason aWhy)
> > >  {
> > > +  RefPtr<Runnable> runnable = NS_NewRunnableMethod(this, &ContentBridgeChild::DeferredDestroy);
> > > +  MessageLoop::current()->PostTask(runnable.forget());
> > 
> > I'm starting to think NS_New*RunnableMethod should just return an
> > already_AddRefed, so we can make this sort of stuff shorter.  Dollars to
> > donuts somebody is going to complain about this when you announce the
> > changes, if they haven't already.  This can be a followup.
> 
> :)

khuey, responding to review comments before they're written.

> > ::: dom/ipc/ContentBridgeParent.h
> > @@ +81,5 @@
> > >  
> > > +  void Close()
> > > +  {
> > > +    // Trick NS_NewRunnableMethod
> > > +    PContentBridgeParent::Close();
> > 
> > Uh.  This is weird.  Can you elaborate?  Why not cast to
> > PContentBridgeParent when calling NS_NewRunnableMethod?
> 
> NS_NewRunnableMethod(instance, &class::function); binds to the type of
> class, not the type of instance.  So here we do
> NS_NewRunnableMethod(contentBridgeParent, &PContentBridgeParent::Close), and
> it fails to compile because PContentBridgeParent is not a refcounted type.

Ah, fair enough.

> > ::: dom/ipc/TabChild.cpp
> > @@ +604,5 @@
> > >    mAsyncPanZoomEnabled = gfxPlatform::AsyncPanZoomEnabled();
> > >  
> > >    nsWeakPtr weakPtrThis(do_GetWeakReference(static_cast<nsITabChild*>(this)));  // for capture by the lambda
> > >    mSetAllowedTouchBehaviorCallback = [weakPtrThis](uint64_t aInputBlockId,
> > > +                                                   nsTArray<TouchBehaviorFlags>&& aFlags)
> > 
> > So we have a couple of these sorts of changes...can you explain why they're
> > necessary and safe?  Some of them look pretty dodgy.
> 
> Basically this array was being copied at every point and the chromium
> templates were hiding that.  I probably should have broken this out
> separately ...

OK, I don't think my r+ is enough to cover whether they can be Move'd safely.  Can you either make the storage type StoreCopyPassByConstLRef (which I think doesn't require any changes), or get somebody to review the Move()s into the NewRunnableMethod invocations and determine whether they're safe?  I think they're all in APZ code, so perhaps Botond is a good choice.
Flags: needinfo?(khuey)

Comment 19

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/af51821b2fc5
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd833da413ad
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f2f3fcac66b
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a8d57e8fa8e
https://hg.mozilla.org/integration/mozilla-inbound/rev/726f7361e0b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bcb784492bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/85ce8cb0639a
Flags: needinfo?(khuey)
sorry had to back this out for frequent memory leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=26873803&repo=mozilla-inbound
Flags: needinfo?(khuey)

Comment 21

a year ago
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9979c96310a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6884584fa07
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bd91d8dd4f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/c70220f1b234
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e6e8bbf54ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/7755d12b8f1c
https://hg.mozilla.org/integration/mozilla-inbound/rev/4026758d1d02
Depends on: 1269268

Comment 22

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf5001ed992f
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8b8d67ee833
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f23832812fd
https://hg.mozilla.org/integration/mozilla-inbound/rev/25c97bee4358
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ae434c0a2c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/67b25748b63a
https://hg.mozilla.org/integration/mozilla-inbound/rev/114ca1fc9c51

Comment 23

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bf5001ed992f
https://hg.mozilla.org/mozilla-central/rev/c8b8d67ee833
https://hg.mozilla.org/mozilla-central/rev/9f23832812fd
https://hg.mozilla.org/mozilla-central/rev/25c97bee4358
https://hg.mozilla.org/mozilla-central/rev/4ae434c0a2c8
https://hg.mozilla.org/mozilla-central/rev/67b25748b63a
https://hg.mozilla.org/mozilla-central/rev/114ca1fc9c51
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1270631
Flags: needinfo?(khuey)
Created attachment 8749489 [details] [diff] [review]
Fix IPDL tests
Attachment #8749489 - Flags: review?(wmccloskey)
Attachment #8749489 - Flags: review?(wmccloskey) → review+

Comment 25

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/277692603a94

Comment 26

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/277692603a94
Depends on: 1273411
Depends on: 1273520
You need to log in before you can comment on or make changes to this bug.