Closed Bug 1312030 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: WebRTC: Audio/Video, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: gkrizsanits, Assigned: jib)

References

Details

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

Attachments

(2 files)

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.
Blocks: e10s-multi
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
(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)
(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
(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
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)
Hi Gabor, I just added a patch I think should solve it.
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 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+
Pushed by jbruaroey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/233a212baec5
Remove MediaParent singleton to work with multi content processes. r=jesup
https://hg.mozilla.org/mozilla-central/rev/233a212baec5
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.