Closed Bug 1268313 Opened 8 years ago Closed 8 years ago

Consolidate on one set of runnable method/function macros

Categories

(Core :: IPC, defect)

Product:
Core
Component:
IPC
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: khuey, Assigned: khuey)

References

(Blocks 1 open bug)

Details

(Whiteboard: btpp-active)

Attachments

(8 files)

Part 1: Be explicit about which NewRunnableMethod callers want CancelableRunnables
14.20 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
Part 2: Replace some NewRunnableMethods with NS_NewNonOwningRunnableMethod
28.15 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
Part 3: Replace some NewRunnableMethods with NS_NewNonOwningCancelableRunnableMethod
17.49 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
Part 4: Replace NewCancelableRunnableMethod
11.19 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
Part 5: Make NS_NewRunnableMethod able to call const functions
3.90 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
Part 6: Replace NewRunnableMethod
74.42 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
Part 7: Move NS_NewRunnableMethod* to mozilla::NewRunnableMethod
479.57 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
Fix IPDL tests
20.29 KB, patch
billm
: review+
Details | Diff | Splinter Review 
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.
Like part 2 but for cancelable things.  After this there are no uses of RunnableMethodTraits left.
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)
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)
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)
Depends on: 1270631
Flags: needinfo?(khuey)
Attachment #8749489 - Flags: review?(wmccloskey) → review+
Depends on: 1273520
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: