Closed Bug 1077133 Opened 5 years ago Closed 5 years ago

[EME] GMPStorage needs be open records exclusively with multiple same-origin Firefox instances

Categories

(Core :: Audio/Video, defect)

x86_64
Windows Vista
defect
Not set

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: cpearce, Assigned: jwwang)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

If we have multiple separate Firefox instances open, and we have two same-origin EME plugins running in each Firefox instance, we need opening of one record in one instance to lock that record, and make it inaccessible in the other instance.

This is different from making storage work in e10s; in e10s we could just remote all storage operations to the parent process; but we can't in fact rely on their being only one parent process; their could be another Firefox instance with a same-origin EME plugin running.
The code that needs to be modified is in GMPStorageParent and maybe GMPStorageChild. Probably it just needs to open the file in exclusive mode.
We could lock a record on Windows using a named cross process Mutex, via CreateMutex:
http://msdn.microsoft.com/en-nz/library/windows/desktop/ms682411%28v=vs.85%29.aspx

See also:
http://mxr.mozilla.org/mozilla-central/source/ipc/glue/CrossProcessMutex_windows.cpp
http://mxr.mozilla.org/mozilla-central/source/ipc/glue/CrossProcessMutex_posix.cpp

There's a posix version too, and it looks like you can name pthread_mutext_t's, it looks like that's supposed to work across processes too.
JW: Would you be able to take this?
Flags: needinfo?(jwwang)
Sure. This bug doesn't seem to require deep know-how in EME. But rather it is a problem about resource-sharing across multiple processes. I will begin with tracing code of GMPStorageParent/GMPStorageChild to understand what record and storage mean in EME.
Assignee: nobody → jwwang
Flags: needinfo?(jwwang)
This bug is necessary for basic playback of EME video on Windows.
Blocks: eme-m1
It looks like named CrossProcessMutex doesn't work. The parameter is not used at all at http://hg.mozilla.org/mozilla-central/annotate/134d1cfc5c9c/ipc/glue/CrossProcessMutex_posix.cpp#l44.
PR_OpenSemaphore seems to work. But I found not such usage in our gecko code.
We must ensure PR_CloseSemaphore is called properly, otherwise the semaphore is locked forever. Or we can just open the file in exclusive mode to prevent concurrent access. But we have to modify GMPDiskStorage::Read and GMPDiskStorage::Write to close the file at the end of the function to allow access from other Firefox instances. Thoughts?
Flags: needinfo?(cpearce)
What happens if we crash while holding the semaphore? Is the semaphore released?

The record should be locked in between GMPDiskStorage::Open and Close() calls.

Exclusive mode may not be 100% reliable, as we sometimes close the file while the record is "open", in order to truncate it to the length of the write, and during that brief window of being closed, the other instance may open and lock the file...
Flags: needinfo?(cpearce)
(In reply to Chris Pearce (:cpearce) from comment #9)
> What happens if we crash while holding the semaphore? Is the semaphore
> released?
No. That's what I mean the semaphore will be locked forever.
It is awkward but we can open another file in exclusive mode to serve as a lock without wait and notify. Then GMPDiskStorage::Open() from a 2nd process will fail since the guard file is already opened in exclusive mode. I guess that is fine though. The guard file will be closed in GMPDiskStorage::Close and we can handle GMPDiskStorage::Write nicely.

However, we might consider using a database manager like sqlite to handle such problems.
We can just have a per-node id file which we lock to prevent concurrent access. I recall Adobe saying they did this with flash, so we should be ok to do the same here.
a per-node id file... since we can have multiple records under a node id, won't that result in only one record can be accessed?
Yes. This is an edge case. So I think that's ok.
Try: https://tbpl.mozilla.org/?tree=Try&rev=e6804ebf2fae

It works fine on other platforms but breaks gtest on windows.

This is because _PR_MD_TLOCKFILE [1] is not implemented. I try to build it on my windows 7 virtual box and find out w95io.c is compiled instead of ntio.c. This is suspicious.

[1] http://hg.mozilla.org/mozilla-central/annotate/7d17b594834f/nsprpub/pr/src/md/windows/w95io.c#l970
Attachment #8526570 - Flags: feedback?(cpearce)
I just realised, I don't think we actually need to fix this.

While looking for an answer to your f?, I found the profile lock code:
http://mxr.mozilla.org/mozilla-central/source/profile/dirserviceprovider/nsProfileLock.cpp#611

Which made me realise that since we store the GMP storage data *inside* the user's profile dir, and we have a runtime restriction that enforces that only one gecko instance can run using a profile at once (by using a lock file no less), we actually don't have a problem here; the existing profile lock ensures that we can't have multiple gecko instances accessing the same GMP storage location at once.

JW: Does that make sense?
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WORKSFORME
Attachment #8526570 - Flags: feedback?(cpearce)
And JW: sorry for wasting your time on this!
(In reply to Chris Pearce (:cpearce) from comment #16)
> I just realised, I don't think we actually need to fix this.
> 
> While looking for an answer to your f?, I found the profile lock code:
> http://mxr.mozilla.org/mozilla-central/source/profile/dirserviceprovider/
> nsProfileLock.cpp#611
> 
> Which made me realise that since we store the GMP storage data *inside* the
> user's profile dir, and we have a runtime restriction that enforces that
> only one gecko instance can run using a profile at once (by using a lock
> file no less), we actually don't have a problem here; the existing profile
> lock ensures that we can't have multiple gecko instances accessing the same
> GMP storage location at once.
> 
> JW: Does that make sense?

IIRC, I was able to launch 2 firefox instances with the same profile directory. (see https://support.mozilla.org/en-US/kb/profiles-where-firefox-stores-user-data to check where the profile folder is)
(In reply to JW Wang [:jwwang] from comment #18)
> IIRC, I was able to launch 2 firefox instances with the same profile
> directory.

How? If I run Firefox with -p command line option and select a profile that is already in use I get an error "Nightly cannot use the profile "$profileName" because it is in use. To continue, close the running instance of Nightlr or choose a different profile".

How did you run the same profile from multiple instances?
(In reply to Chris Pearce (:cpearce) from comment #17)
> And JW: sorry for wasting your time on this!

It is fine :). I still got the chance to trace the code to understand EME/GMP better. Btw, I filed bug 1102709 to fix the problem described in comment 15 to implement _PR_MD_TLOCKFILE for w95io.c. I will put this bug in pending while the fix to bug 1102709 is still ongoing. So if we need this bug again, we have all the tools to implement per-node lock.
Attached image Screenshot.png
Screenshot for 2 firefox instances and their profile folders respectively. I just clicked the app icon in system tray to launch firefox twice and found them share the same profile folder. I wonder if version 28.0 is outdated.
That's probably two Firefox Windows from a single instance. A single Firefox process can have multiple active Windows that all run under the same process. If you look at the process list running on your system, do you see multiple "firefox" processes?
Oh yeah. I see only one firefox process. Thanks for the clarification.
No longer blocks: eme-m1
Mass update firefox-status to track EME uplift.
No longer blocks: 1032660
You need to log in before you can comment on or make changes to this bug.