Closed Bug 1481350 Opened Last year Closed Last year

Don't try to handle messages sent to child-allocated browsers

Categories

(Core :: Web Replay, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: bhackett, Assigned: bhackett)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
PBrowser instances can be allocated on either the parent and the child side.  When the parent allocates a PBrowserParent then both the middleman and recording child will allocate the corresponding PBrowserChild, but if the recording process allocates a PBrowserChild then the middleman will not end up with a corresponding actor.  If the UI process tries to send a message to one of the actors which is only in the recording child then the middleman needs to make sure it does not handle the message itself.  Testing the routing ID in this way is pretty gross but I didn't see a better way to check for this.
Attachment #8998072 - Flags: review?(continuation)
Comment on attachment 8998072 [details] [diff] [review]
patch

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

Hmm I don't know too much about this routing ID stuff. It looks like the logic for this happens in IToplevelProtocol::ToplevelState::Register(), and thus it is derived from mProtocol->GetSide()? Can't you then instead just check aSide?

Nathan might be more familiar with routing IDs. If not I can dig into it some more.

::: toolkit/recordreplay/ipc/ParentForwarding.cpp
@@ +116,5 @@
>        // Teardown that must be performed in both processes.
>        type == dom::PBrowser::Msg_Destroy__ID) {
> +    if (type >= dom::PBrowser::PBrowserStart &&
> +        type <= dom::PBrowser::PBrowserEnd &&
> +        aMessage.routing_id() < 0) {

It looks like this will also catch MSG_ROUTING_NONE. Is that ok?
Attachment #8998072 - Flags: review?(nfroyd)
(In reply to Andrew McCreight [:mccr8] from comment #1)
> Comment on attachment 8998072 [details] [diff] [review]
> patch
> 
> Review of attachment 8998072 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hmm I don't know too much about this routing ID stuff. It looks like the
> logic for this happens in IToplevelProtocol::ToplevelState::Register(), and
> thus it is derived from mProtocol->GetSide()? Can't you then instead just
> check aSide?

This should work at the point where the actor is being registered, but after that point the protocol in use could be operating on actors that were allocated by either side.

> ::: toolkit/recordreplay/ipc/ParentForwarding.cpp
> @@ +116,5 @@
> >        // Teardown that must be performed in both processes.
> >        type == dom::PBrowser::Msg_Destroy__ID) {
> > +    if (type >= dom::PBrowser::PBrowserStart &&
> > +        type <= dom::PBrowser::PBrowserEnd &&
> > +        aMessage.routing_id() < 0) {
> 
> It looks like this will also catch MSG_ROUTING_NONE. Is that ok?

This should be OK: MessageChannel::Send will fail if the message's routing ID is MSG_ROUTING_NONE.
Comment on attachment 8998072 [details] [diff] [review]
patch

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

I think this makes sense?

mccr8's idea should work, right, because if aSide == ParentSide, then aMessage.routing_id() must be < 0?  I'd prefer checking aSide--possibly with an assert on routing_id--and if that can't work, perhaps we should add a fromParent/fromChild method to Message so people don't have to futz with routing_id().

::: toolkit/recordreplay/ipc/ParentForwarding.cpp
@@ +116,5 @@
>        // Teardown that must be performed in both processes.
>        type == dom::PBrowser::Msg_Destroy__ID) {
> +    if (type >= dom::PBrowser::PBrowserStart &&
> +        type <= dom::PBrowser::PBrowserEnd &&
> +        aMessage.routing_id() < 0) {

Why are we not doing the same thing for the PContent messages?  I guess we wouldn't allocate them on either side, because PContent is a toplevel protocol, so there's no chance of getting confused?

And do the PCompositorBridge messages down below need similar treatment?
Attachment #8998072 - Flags: review?(nfroyd) → review+
Attachment #8998072 - Flags: review?(continuation)
Yeah, if the < 0 stuff is needed, there really needs to be some kind of abstraction. Nathan's suggestion sounds reasonable to me.
(In reply to Nathan Froyd [:froydnj] from comment #3)
> Comment on attachment 8998072 [details] [diff] [review]
> patch
> 
> Review of attachment 8998072 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this makes sense?
> 
> mccr8's idea should work, right, because if aSide == ParentSide, then
> aMessage.routing_id() must be < 0?  I'd prefer checking aSide--possibly with
> an assert on routing_id--and if that can't work, perhaps we should add a
> fromParent/fromChild method to Message so people don't have to futz with
> routing_id().

In this method, aSide indicates which direction we're forwarding the message (UI process to recording child process, or vice versa); at this point of execution aSide is always Parent (the message is being forwarded from the UI process to the recording child).  The sign of the routing ID indicates which side originally allocated the actor which the message is associated with.  A channel can send the same messages for an actor regardless of where it was originally allocated, and at this point we could be seeing (for example) a PShow message from the UI process to show a browser that was allocated on either the parent side or the child side.

Would it be clearer here if we kept a list of all the browsers which have been allocated within the middleman, and check against that list instead of the routing ID?  This requires more bookkeeping but is more robust and straightforwardly correct.

> ::: toolkit/recordreplay/ipc/ParentForwarding.cpp
> @@ +116,5 @@
> >        // Teardown that must be performed in both processes.
> >        type == dom::PBrowser::Msg_Destroy__ID) {
> > +    if (type >= dom::PBrowser::PBrowserStart &&
> > +        type <= dom::PBrowser::PBrowserEnd &&
> > +        aMessage.routing_id() < 0) {
> 
> Why are we not doing the same thing for the PContent messages?  I guess we
> wouldn't allocate them on either side, because PContent is a toplevel
> protocol, so there's no chance of getting confused?
> 
> And do the PCompositorBridge messages down below need similar treatment?

PContent and PCompositorBridge are always allocated by a particular side, so don't run into this problem.
Attached patch patchSplinter Review
IPDL actors actually already have methods to find the children they are managing, so this patch uses that to ignore messages sent to actors which do not exist in the middleman process.
Attachment #8998072 - Attachment is obsolete: true
Attachment #9002525 - Flags: review?(nfroyd)
Comment on attachment 9002525 [details] [diff] [review]
patch

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

Ugh, this is worse than the previous patch, but I guess it is more correct?  I suppose trying to make PBrowser consistently allocated on one side would regress page load, or something, and be quite difficult in any event...
Attachment #9002525 - Flags: review?(nfroyd) → review+
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8049cc432128
Don't try to handle messages sent to child-allocated browsers, r=froydnj.
https://hg.mozilla.org/mozilla-central/rev/8049cc432128
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.