Closed Bug 1087901 Opened 7 years ago Closed 6 years ago

[EME] GMP on MacOSX is instantiated twice

Categories

(Core :: Audio/Video: Playback, defect)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: cpearce, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Attached patch PatchSplinter Review
Due to changes in bug 1060179 I inadvertently made it so that we call GMPChild::LoadPluginLibrary twice on MacOSX; firstly when GMPChild::OnChannelConnected() is called, and again when GMPChild::RecvStartPlugin() is called.

I discovered this in bug 1055395 when I was writing gTests which failed on MacOSX due to this.

I had a patch in bug 1055395, re-attached here, but Steven Michaud wants time to test it, so I spun off this bug to track fixing this issue.
Comment on attachment 8510109 [details] [diff] [review]
Patch

This patch now looks fine to me.

When I first saw it at bug 1055395, I misunderstood what it does.  I thought it undid the careful work I'd done to guarantee that, on the Mac, the sandbox doesn't get started until IPC has been fully set up.  I was wrong about that.

As I say at bug 1055395 comment #9, I've been using a call to GMPChild::OnChannelConnected() to indicate that IPC *must* now have been completely set up.  But after digging around in the IPC code, I discovered that the receipt of *any* IPC call will guarantee this just as well -- including one that results in a call to GMPChild::ReceiveStartPlugin().

Listener::OnChannelConnected() is called here, on receipt of a "hello" message:

https://hg.mozilla.org/mozilla-central/annotate/d8de0d7e52e0/ipc/chromium/src/chrome/common/ipc_channel_posix.cc#l617

There's nothing special about this message (compared to any other), and no guarantee that it will be the first to arrive on a particular channel.  So receiving it offers no better guarantee that IPC is fully set up than receiving any other message.
Attachment #8510109 - Flags: review+
Thanks for digging around and confirming that Steven!
Component: Audio/Video → Audio/Video: Playback
I'm guessing this made it to m-c.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.