933821 - IPC: ###!!! ABORT: need a route: file ../../../../mozilla-central/ipc/glue/MessageChannel.cpp

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: fuzzing-session

Tested with an opt/non-debug build of https://github.com/posidron/mozilla-central/commit/26121cb

Comment 1

[Josh Matthews [:jdm]]

This should be a simple matter of changing |IPC_ASSERT(MSG_ROUTING_NONE != msg->routing_id(), "need a route");| into a call to PrintErrorMessage and return false.

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Yes please!

Comment 3

[Peiyong Lin[:lpy](UTC+8)]

I found the code related in http://mxr.mozilla.org/mozilla-central/source/ipc/glue/MessageChannel.cpp#216 and http://mxr.mozilla.org/mozilla-central/source/ipc/glue/MessageChannel.cpp#238, I will fix it soon.

Comment 4

[Peiyong Lin[:lpy](UTC+8)]

which kind of channel name should I use?

Comment 5

[Josh Matthews [:jdm]]

Probably just MessageChannel::Send or something.

Comment 6

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug933821.patch

Sorry for so late. :)

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 826443 [details] [diff] [review]
bug933821.patch

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

::: ipc/glue/MessageChannel.cpp
@@ +214,5 @@
>      AssertWorkerThread();
>      mMonitor->AssertNotCurrentThreadOwns();
> +    if (MSG_ROUTING_NONE == msg->routing_id()) {
> +        PrintErrorMessage(mSide, "MessageChannel::Echo", "need a route");
> +        return false;

Hm, I don't think returning false is enough here... The other spots call ReportConnectionError, and that notifies mListener. Shouldn't that happen here (and below)?

Comment 8

[Peiyong Lin[:lpy](UTC+8)]

You mean I should call ReportConnectionError right after PrintErrorMessage, and then return false? I will generate another patch soon.

Comment 9

[Josh Matthews [:jdm]]

I suspect that calling ReportConnectionError won't work, since we'll end up in the default case and abort.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Right, calling ReportConnectionError isn't what's needed (there are locking concerns too). The thing I'm wondering is if mListener needs to be notified. I suspect yes, but I don't have time to check at the moment.

Comment 11

[Peiyong Lin[:lpy](UTC+8)]

Should I update my patch, adding mListener?

Comment 12

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug933821-v2.patch

v2

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 827426 [details] [diff] [review]
bug933821-v2.patch

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

This looks pretty good! I'd suggest a few changes:

::: ipc/glue/MessageChannel.cpp
@@ +207,5 @@
>      mSide = aSide;
>  }
>  
> +static void
> +PrintErrorMessage(Side side, const char* channelName, const char* msg);

This shouldn't be needed, just move the function implementation to the top of the file right after the IPC_ASSERT macro.

@@ +210,5 @@
> +static void
> +PrintErrorMessage(Side side, const char* channelName, const char* msg);
> +
> +void
> +MessageChannel::ReportMessageRouteError(const char* channelName)

Let's move this next to ReportConnectionError.

@@ +224,5 @@
>      AssertWorkerThread();
>      mMonitor->AssertNotCurrentThreadOwns();
> +    if (MSG_ROUTING_NONE == msg->routing_id()) {
> +        ReportMessageRouteError("MessageChannel::Echo");
> +        ReportConnectionError("MessageChannel::Echo");

There's no need to call both here, and in fact it will cause problems if mListener is notified twice. Just call ReportMessageRouteError. Same for MessageChannel::Send.

::: ipc/glue/MessageChannel.h
@@ +79,5 @@
>      // for process links only, not thread links.
>      void CloseWithError();
>  
> +    // Report Message MSG_ROUTING_NONE error.
> +    void ReportMessageRouteError(const char* channelName);

Let's move this next to ReportConnectionError (in the private section), and make it const too.

Comment 14

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug933821-v2.patch

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 831305 [details] [diff] [review]
bug933821-v2.patch

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

This looks great, thanks!

Comment 16

[Josh Matthews [:jdm]]

Peiyong, if you can attach a patch with a more descriptive commit message (including the r=bent), then this is ready to be checked in.

Comment 17

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug933821.patch

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/217fe447e1ac

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/217fe447e1ac