Closed Bug 1573381 Opened 5 months ago Closed 5 months ago

gtest MediaMIMETypes.MediaCodecs fails: Bug in MediaCodecs::ContainsPrefix()


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

68 Branch



Tracking Status
firefox-esr68 --- fixed
firefox70 --- fixed


(Reporter: msirringhaus, Assigned: msirringhaus)




(1 file)

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.

OS: Unspecified → Linux
Hardware: Unspecified → x86_64

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:
In the meantime, moving this over to a component so a developer can check out the attached patch.
Thanks for the report!

Component: Untriaged → Audio/Video: Playback
Flags: needinfo?(msirringhaus)
Product: Firefox → Core

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

Flags: needinfo?(msirringhaus)

NI: me to remind myself to look at this when I have a moment.

Flags: needinfo?(bvandyk)
Comment on attachment 9084983 [details] [diff] [review]

LGTM. Looks like something we should be able to expand the tests to cover as well -- I can do that as a follow up.
Flags: needinfo?(bvandyk)
Attachment #9084983 - Flags: review+
Ever confirmed: true
Priority: -- → P3

Requesting check in -- this fixes an incorrect comparison when checking MimeType prefixes that leads to overly permissive matching.

Keywords: checkin-needed

Pushed by
"gtest MediaMIMETypes.MediaCodecs fails: Bug in MediaCodecs::ContainsPrefix()". r=bvandyk

Keywords: checkin-needed
See Also: → 1577021
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Assignee: nobody → msirringhaus

Once it has been in central long enough, would an uplift to 68esr be possible?

Flags: needinfo?(bvandyk)

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

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.MediaCodecs gtest.
  • 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.
Flags: needinfo?(bvandyk)
Attachment #9084983 - Flags: approval-mozilla-esr68?
Comment on attachment 9084983 [details] [diff] [review]

Fixes a test failure, approved for 68.2esr.
Attachment #9084983 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
You need to log in before you can comment on or make changes to this bug.