Closed Bug 933821 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: IPC, defect, critical)

ARM
Gonk (Firefox OS)
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: posidron, Assigned: lpy)

References

(Blocks 1 open bug)

Details

(Keywords: crash, Whiteboard: [mentor=jdm][lang=c++][good first bug])

Attachments

(1 file, 4 obsolete files)

Attached file fuzzing-session (obsolete) —
Tested with an opt/non-debug build of https://github.com/posidron/mozilla-central/commit/26121cb
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.
Whiteboard: [mentor=jdm][lang=c++][good first bug]
which kind of channel name should I use?
Assignee: nobody → pylaurent1314
Probably just MessageChannel::Send or something.
Attached patch bug933821.patch (obsolete) — Splinter Review
Sorry for so late. :)
Attachment #826443 - Flags: review?(bent.mozilla)
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)?
You mean I should call ReportConnectionError right after PrintErrorMessage, and then return false? I will generate another patch soon.
I suspect that calling ReportConnectionError won't work, since we'll end up in the default case and abort.
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.
Should I update my patch, adding mListener?
Attached patch bug933821-v2.patch (obsolete) — Splinter Review
v2
Attachment #826443 - Attachment is obsolete: true
Attachment #826443 - Flags: review?(bent.mozilla)
Attachment #827426 - Flags: review?(bent.mozilla)
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.
Attachment #827426 - Flags: review?(bent.mozilla)
Attached patch bug933821-v2.patch (obsolete) — Splinter Review
Attachment #825977 - Attachment is obsolete: true
Attachment #827426 - Attachment is obsolete: true
Attachment #831305 - Flags: review?(bent.mozilla)
Comment on attachment 831305 [details] [diff] [review]
bug933821-v2.patch

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

This looks great, thanks!
Attachment #831305 - Flags: review?(bent.mozilla) → review+
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.
Attached patch bug933821.patchSplinter Review
Attachment #831305 - Attachment is obsolete: true
Attachment #832057 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/217fe447e1ac
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.