Closed Bug 1266595 Opened 8 years ago Closed 8 years ago

Replace Task with Runnable, CancelableTask with CancelableRunnable


(Core :: IPC, defect)

Not set



Tracking Status
firefox49 --- fixed


(Reporter: khuey, Assigned: khuey)


(Blocks 1 open bug)


(Whiteboard: btpp-active)


(3 files)

Attached patch PatchSplinter Review
I don't have any ideas for breaking this up.  Most of it is mechanical, centered around renaming the base classes, changing function signatures so they actually override, and adding reference counting.  The most interesting parts are in ipc/
Attachment #8744105 - Flags: review?(nfroyd)
Whiteboard: btpp-active
Comment on attachment 8744105 [details] [diff] [review]

Review of attachment 8744105 [details] [diff] [review]:

I think a productive way to break this up for review purposes would have been:

part 1: a patch to ipc/ for the core changes
part 1a: a patch to ipc/ that does renaming for clients and such (perhaps rolled into part 2?)
part 2: fix all the rest of Gecko
part 3: deleting the object tracking stuff

even if you landed all the patches in a single go.  Just for clarification: the core changes are task.h and message_loop.{cc,h}; I think everything beyond that is mechanical, right?

::: dom/ipc/ContentParent.cpp
@@ +2002,5 @@
>  DelayedDeleteSubprocess(GeckoChildProcessHost* aSubprocess)
>  {
> +  RefPtr<DeleteTask<GeckoChildProcessHost>> task =
> +    new DeleteTask<GeckoChildProcessHost>(aSubprocess);
> +  XRE_GetIOMessageLoop()->PostTask(task.forget());


is shorter here and throughout if you like that kind of thing.

::: ipc/chromium/src/base/
@@ +247,1 @@
>    deferred_non_nestable_work_queue_.pop();

Bonus points in avoiding extraneous refcounting here.

@@ +409,2 @@
>        PendingTask pending_task = work_queue_.front();
>        work_queue_.pop();

Are we now doing more refcounting here due to copying PendingTask?  Maybe we need a move constructor for PendingTask?

@@ +436,4 @@
>    }
>    PendingTask pending_task =;
>    delayed_work_queue_.pop();

Same thing here?

::: ipc/chromium/src/base/task.h
@@ +155,5 @@
>    }
>   protected:
>    template <class Method, class Params>
> +  class RunnableMethod : public mozilla::Runnable {

Can you file and/or do a followup bug for ripping out all the things we can now replace with Gecko equivalents from nsThreadUtils.h or similar?
Attachment #8744105 - Flags: review?(nfroyd) → review+
Attached patch Review commentsSplinter Review
Something like this?
Attachment #8746099 - Flags: review?(nfroyd)
Comment on attachment 8746099 [details] [diff] [review]
Review comments

Review of attachment 8746099 [details] [diff] [review]:

r=me with changing MessageLoop::Do{Delayed,}Work to use Moves in conjuction with front() and top().  I think that should be all that's needed, yell at me if it's not.

::: ipc/chromium/src/base/message_loop.h
@@ +295,5 @@
> +          sequence_num(aOther.sequence_num),
> +          nestable(aOther.nestable) {
> +    }
> +
> +    // std::queue<T>::front/pop are dumb, so we have to have this.

What does this even mean?  front returns T&, and pop doesn't return anything.  The right thing to do here is to write the code like:

  PendingTask pt = Move(mQueue.front());

and not have the copy constructors at all.
Attachment #8746099 - Flags: review?(nfroyd) → review+
Depends on: 1268432
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
This broke IPDL unit tests. Can you please fix Kyle?
Flags: needinfo?(khuey)
How does one run the IPDL unit tests?
Flags: needinfo?(khuey) → needinfo?(wmccloskey)
Compile with ac_add_options --enable-ipdl-tests and you'll get build errors.
Flags: needinfo?(wmccloskey) → needinfo?(khuey)
Attached patch IPDL TestsSplinter Review
There are a couple minor modifications to make these build with --warnings-as-errors (at least on my machine) too.
Flags: needinfo?(khuey)
Attachment #8748466 - Flags: review?(wmccloskey)
Comment on attachment 8748466 [details] [diff] [review]
IPDL Tests

Review of attachment 8748466 [details] [diff] [review]:


::: ipc/ipdl/test/cxx/TestRaceDeadlock.cpp
@@ +5,5 @@
>  // #define TEST_TIMEOUT 5000
>  using namespace mozilla::ipc;
>  typedef mozilla::ipc::MessageChannel::Message Message;
> +typedef mozilla::ipc::MessageChannel::MessageInfo MessageInfo;

what's this?
Attachment #8748466 - Flags: review?(wmccloskey) → review+
Depends on: 1275299
Depends on: 1281695
You need to log in before you can comment on or make changes to this bug.