gtest MediaMIMETypes.MediaCodecs fails: Bug in MediaCodecs::ContainsPrefix()
Categories
(Core :: Audio/Video: Playback, defect, P3)
Tracking
()
People
(Reporter: msirringhaus, Assigned: msirringhaus)
References
Details
Attachments
(1 file)
|
541 bytes,
patch
|
bryce
:
review+
RyanVM
:
approval-mozilla-esr68+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0
Steps to reproduce:
Run gtest MediaMIMETypes.MediaCodecs.
Test fails on big endian machines, but the underlying bug affects in theory all archs.
Actual results:
Test fails.
If I have understood it correctly, the function searches for a given prefix. For this it gets the length of the prefix and does a memcmp() of that size.
But the underlying string is of type char16_t, so the number of characters is not the number of bytes.
For a single character prefix, only one of the 2 bytes are compared.
On big endian, this leads to false positives.
Expected results:
Test succeeds.
|
Assignee | |
Updated•5 years ago
|
|
||
Comment 1•5 years ago
|
||
Hi msirringhaus,
Can you provide more details steps on how to run the gtest?
Is this issue reproducible on the latest Nightly 70 too? Here is a link from where you can download and try it out: https://www.mozilla.org/en-US/firefox/channel/desktop/
In the meantime, moving this over to a component so a developer can check out the attached patch.
Thanks for the report!
|
|
Assignee | |
Comment 2•5 years ago
|
||
It should be just ./mach gtest MediaMIMETypes.MediaCodecs after building.
Note this test only fails on big endian machine, but the underlying bug affects all archs, as far as I can see.
The relevant code has not changed in mozilla-central, so the bug is there as well (if it is indeed a bug).
NI: me to remind myself to look at this when I have a moment.
Comment on attachment 9084983 [details] [diff] [review] mozilla-media-mime-types.patch LGTM. Looks like something we should be able to expand the tests to cover as well -- I can do that as a follow up.
Requesting check in -- this fixes an incorrect comparison when checking MimeType prefixes that leads to overly permissive matching.
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3d47c2d160a7
"gtest MediaMIMETypes.MediaCodecs fails: Bug in MediaCodecs::ContainsPrefix()". r=bvandyk
|
||
Comment 7•5 years ago
|
||
| bugherder | ||
|
||
Updated•5 years ago
|
|
|
Assignee | |
Comment 8•5 years ago
|
||
Once it has been in central long enough, would an uplift to 68esr be possible?
(In reply to msirringhaus from comment #8)
Once it has been in central long enough, would an uplift to 68esr be possible?
Sounds reasonable. Holding NI to remind myself to request uplift after this has baked for awhile.
Bug 1577021 is tracking adding tests for this.
Comment on attachment 9084983 [details] [diff] [review]
mozilla-media-mime-types.patch
ESR Uplift Approval Request
- If this is not a sec:{high,crit} bug, please state case for ESR consideration: Developer ergonomics: this fixes a bug that causes our gtests to fail on big endian machines (and on all machines following Bug 1577021 landing).
- User impact if declined: Big endian machines will see permafails for the
MediaMIMETypes.MediaCodecsgtest. - Fix Landed on Version: 70
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): It's a one line change in a function that we do not appear to use outside of tests (currently).
- String or UUID changes made by this patch: None.
|
||
Comment 12•5 years ago
|
||
Comment on attachment 9084983 [details] [diff] [review] mozilla-media-mime-types.patch Fixes a test failure, approved for 68.2esr.
|
||
Comment 13•5 years ago
|
||
| bugherder uplift | ||
Description
•