sIPCServingParent does not seem to be multi-content-process e10s compatible

RESOLVED FIXED in Firefox 52

Status

()

Core
WebRTC: Audio/Video
P1
normal
Rank:
10
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Gabor Krizsanits (INACTIVE), Assigned: jib)

Tracking

(Blocks: 1 bug)

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [e10s-multi:M?])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

a year ago
I've run into this problem while working on enabling 2 content processes. What if there are two content processes that wants to use MediaChild? On parent side we seem to have a singleton (sIPCServingParent)... how is this supposed to work? For the second child when it send SendPMediaConstructor this will just assert in debug build and probably corrupt memory on release build right now.
(Reporter)

Updated

a year ago
Blocks: 1207306
Flags: needinfo?(jib)
Whiteboard: [e10s-multi:M?]
Can you point me to an example of the prevailing pattern to use here? I'd be happy to update this to use it. Thanks.
Flags: needinfo?(jib) → needinfo?(gkrizsanits)
Component: WebRTC → WebRTC: Audio/Video
Rank: 25
Priority: -- → P2
(Reporter)

Comment 3

a year ago
(In reply to Jan-Ivar Bruaroey [:jib] from comment #2)
> Can you point me to an example of the prevailing pattern to use here? I'd be
> happy to update this to use it. Thanks.

Thanks a lot in advance, and sorry about the lag with the reply. I'm not sure why is sIPCServingParent a singleton to start with. The usual pattern is pretty simple like PWebrtcGlobal which is also managed by PContent: [1]. This static variable's (sIPCServingParent) only purpose seems to be to ensure that there is only one instance is created on parent side and not one for each content process. I have no idea why we want that, I've seen Bill reviewing some code in this area so I'm needinfo-ing him, he might remember the background story here, or has an idea about the way forward.

Bill, do you know why are we enforcing a singleton here?

[1] : http://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.cpp#782
Flags: needinfo?(gkrizsanits) → needinfo?(wmccloskey)
I'm not familiar with this code. It's very hard to read. It seems like the only place we use this singleton is in Parent<Super>::RecvGetOriginKey. It seems like the singleton being used in this case is just... |this|? Maybe I'm wrong though. It's really hard to figure out what's going on.
Flags: needinfo?(wmccloskey)
(Reporter)

Comment 5

a year ago
(In reply to Bill McCloskey (:billm) from comment #4)
> I'm not familiar with this code. It's very hard to read. It seems like the

Yeah it's weird, sorry for bothering you with it then...

> only place we use this singleton is in Parent<Super>::RecvGetOriginKey. It
> seems like the singleton being used in this case is just... |this|? Maybe
> I'm wrong though. It's really hard to figure out what's going on.

Yeah, it seems like the e10s version of this class also maintains a non-e10s
version as a member and the non10s version instead of having a reference back
to the e10s parent uses this weird static variable. Maybe because other threads
are involved? But I'm a bit confused here so might be totally wrong.

So I think that we should create one e10s and non-e10s parent for each content
processes and find another way for interaction between them than this static
variable. But I'm not sure why we need both on the first place, or if the
non-e10s class has to be a singleton for some reason... so it's hard to tell.

Jan-Ivar, do you know why we need this static variable exactly?
Flags: needinfo?(jib)
Hi Gabor, I'm familiar with this code, no worries. Using a different ipc parent for each content process should work fine (the nonE10s is just for requests from non-e10s windows).
Assignee: nobody → jib
Flags: needinfo?(jib)
Summary: sIPCServingParent does not seem to be e10s compatible → sIPCServingParent does not seem to be multi-process e10s compatible
Summary: sIPCServingParent does not seem to be multi-process e10s compatible → sIPCServingParent does not seem to be multi-content-process e10s compatible
(Reporter)

Comment 7

a year ago
(In reply to Jan-Ivar Bruaroey [:jib] from comment #6)
> Hi Gabor, I'm familiar with this code, no worries. Using a different ipc
> parent for each content process should work fine (the nonE10s is just for
> requests from non-e10s windows).

Awesome, thanks! So it seems like this is the few last blocker to turn on e10s-multi on nightly... Do you by any chance have an ETA on this bug? We are going to turn off a few tests initially that were failing and we have a few known issues (service workers, session storage) so in general it wouldn't be the end of the word if a few corner cases would be buggy initially, but with this bug I'm a bit hesitant because I'm afraid that this pointer could cause a security risk, but at very least some crashes...
Rank: 25 → 10
Priority: P2 → P1
(Reporter)

Comment 8

a year ago
Created attachment 8806347 [details] [diff] [review]
part1: prevent the allocation of a 2nd PMedia parent

Until this bug is fixed I would like to force single content process usage for the failing test and make sure that we never set sIPCServingParent twice in release build. I'm trying to enable 2 content processes on nightly this week and this is the last remaining blocker.
Attachment #8806347 - Flags: review?(jib)
Comment hidden (mozreview-request)
Hi Gabor, I just added a patch I think should solve it.
(Reporter)

Comment 11

a year ago
Comment on attachment 8806347 [details] [diff] [review]
part1: prevent the allocation of a 2nd PMedia parent

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

Even better, thanks!
Attachment #8806347 - Flags: review?(jib)

Comment 12

a year ago
mozreview-review
Comment on attachment 8806370 [details]
Bug 1312030 - Remove MediaParent singleton to work with multi content processes.

https://reviewboard.mozilla.org/r/89832/#review89636
Attachment #8806370 - Flags: review?(rjesup) → review+

Comment 13

a year ago
Pushed by jbruaroey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/233a212baec5
Remove MediaParent singleton to work with multi content processes. r=jesup

Comment 14

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/233a212baec5
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.