[EME] GMPStorage can't handle long record names

RESOLVED FIXED in Firefox 37

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla37
x86_64
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox37 fixed, firefox38 fixed, firefox39 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

3 years ago
Bug 1100499 changed our GMPStorage API to base64encode the record name and uses that as the file name. If a long record name is put through this, it can hit the max path size of a file on Windows, resulting in I/O failure.
(Assignee)

Comment 1

3 years ago
Created attachment 8528754 [details] [diff] [review]
Patch: Detect GMPStorage failure in gtests

We should really detect failures in GMPStorage reads/writes in our gtests, instead of assuming that reads/writes always succeed...
Attachment #8528754 - Flags: review?(edwin)
(Assignee)

Comment 2

3 years ago
Created attachment 8528757 [details] [diff] [review]
Patch: Store GMP record name in record file on disk

* Revert to using Mozilla::HashString(recordName) as the filename for records.
* Write the length of the recordName and then the recordName into the start of the record's file, before the actual record data.
* Treat files which aren't in this format as empty records; so they can be overwritten.
Attachment #8528757 - Flags: review?(rjesup)
(Assignee)

Comment 3

3 years ago
Created attachment 8528787 [details] [diff] [review]
Patch: Store GMP record name in record file on disk

With WAE compile fixes on for GCC, and test tweaked to pass...
Attachment #8528757 - Attachment is obsolete: true
Attachment #8528757 - Flags: review?(rjesup)
Attachment #8528787 - Flags: review?(rjesup)
(Assignee)

Comment 4

3 years ago
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d5124979a26c
https://tbpl.mozilla.org/?tree=Try&rev=d5124979a26c
Attachment #8528754 - Flags: review?(edwin) → review+
Comment on attachment 8528787 [details] [diff] [review]
Patch: Store GMP record name in record file on disk

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

Looks good... spent a while looking over the size/offset stuff; looks good
Attachment #8528787 - Flags: review?(rjesup) → review+
(Assignee)

Comment 6

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=2c9a925932dd
(Assignee)

Comment 7

3 years ago
Created attachment 8531710 [details] [diff] [review]
Patch: Detect GMPStorage failure in gtests

Commit message updated.
Attachment #8531710 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8528754 - Attachment is obsolete: true
(Assignee)

Comment 8

3 years ago
Created attachment 8531712 [details] [diff] [review]
Patch: Store GMP record name in record file on disk

Commit message updated to reflect r+
Attachment #8528787 - Attachment is obsolete: true
Attachment #8531712 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fc3ea149549
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d18f4965871
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5fc3ea149549
https://hg.mozilla.org/mozilla-central/rev/8d18f4965871
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
(Assignee)

Comment 11

3 years ago
Mass update firefox-status to track EME uplift.
status-firefox37: --- → fixed
status-firefox38: --- → fixed
status-firefox39: --- → fixed
You need to log in before you can comment on or make changes to this bug.