1266595 - Replace Task with Runnable, CancelableTask with CancelableRunnable

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

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/

Comment 1

[Nathan Froyd [:froydnj]]

Comment on attachment 8744105 [details] [diff] [review]
Patch

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());

XRE_GetIOMessageLoop()->PostTask(MakeAndAddRef<DeleteTask>(aSubprocess));

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

::: ipc/chromium/src/base/message_loop.cc
@@ +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_.top();
>    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?

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Review comments

Something like this?

Comment 3

[Nathan Froyd [:froydnj]]

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());
  mQueue.pop();

and not have the copy constructors at all.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/30dcf34cfb75

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/30dcf34cfb75

Comment 6

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

This broke IPDL unit tests. Can you please fix Kyle?

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

How does one run the IPDL unit tests?

Comment 8

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

Compile with ac_add_options --enable-ipdl-tests and you'll get build errors.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: IPDL Tests

There are a couple minor modifications to make these build with --warnings-as-errors (at least on my machine) too.

Comment 10

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

Comment on attachment 8748466 [details] [diff] [review]
IPDL Tests

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

Thanks!

::: 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?

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd6d02a7cd01

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dd6d02a7cd01