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

Status: RESOLVED FIXED

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

Description

[Gabor Krizsanits (INACTIVE)]

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.

Comment 1

[Gabor Krizsanits (INACTIVE)]

https://treeherder.mozilla.org/logviewer.html#?job_id=289687&repo=ash#L2422

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 3

[Gabor Krizsanits (INACTIVE)]

(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

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 5

[Gabor Krizsanits (INACTIVE)]

(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?

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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).

Comment 7

[Gabor Krizsanits (INACTIVE)]

(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...

Comment 8

[Gabor Krizsanits (INACTIVE)]

Attachment: 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.

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1312030 - Remove MediaParent singleton to work with multi content processes.

Review commit: https://reviewboard.mozilla.org/r/89832/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/89832/

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Hi Gabor, I just added a patch I think should solve it.

Comment 11

[Gabor Krizsanits (INACTIVE)]

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!

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

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

https://reviewboard.mozilla.org/r/89832/#review89636

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/233a212baec5
Remove MediaParent singleton to work with multi content processes. r=jesup

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/233a212baec5