Closed Bug 1571081 Opened 5 years ago Closed 5 years ago

Attempting to log MediaKeys to console triggers assert

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- fixed
firefox68 --- wontfix
firefox69 --- fixed
firefox70 --- fixed

People

(Reporter: bryce, Assigned: bryce)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files)

Attached file testcase.html

Load the testcase in an assert enabled build and then open the console. This appears related to trying to log the media keys to console.

Edit: it looks like the crash happens when opening the console and attempting to view the value. I suspect there's something going on here with evaluating the value of the keys after they're in some unhappy state.

This is a regression. Smallest window I could find it this. Fairly confident it will have been regressed by bug 1522547. Upping priority as this is a recent regression.

Assignee: nobody → bvandyk
Keywords: regression
Priority: P3 → P2
Regressions: 1522547

Backing out the last changeset from 1522547 alleviates the issue. Looks like my changing of the macros in that patch broke something.

Regressed by: 1522547
No longer regressions: 1522547

This looks to be due to this code only having an entry for nsIDocumentActivity, when it also needs one for nsISupports. I'd based this code on similar MediaRecorder code, but made the mistake of overlooking that the MediaRecorder also has NS_INTERFACE_MAP_END_INHERITING(DOMEventTargetHelper), which takes care of the nsISupports interface case through inheritance.

Bug 1522547 overlooked the need for specifying map entries for both nsISupports
and nsIDocumentActivity and only had the latter. This changeset adds the missing
nsISupports case.

Pushed by bvandyk@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6e741ddf4edf
Make MediaKeys correctly specify a NS_INTERFACE_MAP_ENTRY for nsISupports. r=mccr8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

What's the user impact of this bug? Does this need to be nominated for uplift or can it ride the trains?

Flags: needinfo?(bvandyk)

For me this was only an issue in debug builds due to the assert. In those cases I only saw the issue when trying to interact with media keys via the browser tools debugger or logging the keys to the console. However, I'm unclear of the other scenarios where having botched the query interface may bite us. I had planned to request uplift to beta and ESR on the basis that I'm concerned about such unknown unknowns.

:mccr8, do you have any thoughts about the severity of the missing NS_INTERFACE_MAP_ENTRY(nsISupports) entry?

Flags: needinfo?(bvandyk) → needinfo?(continuation)

This will just make QIing to nsISupports fail. We maybe just never QI these objects to nsISupports under normal circumstances. Uplifting this would be low risk, but also I think it likely doesn't matter in practice, outside of debugging scenarios like you mentioned.

Flags: needinfo?(continuation)

Comment on attachment 9086513 [details]
Bug 1571081 - Make MediaKeys correctly specify a NS_INTERFACE_MAP_ENTRY for nsISupports. r?mccr8

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Developer ergonomics: this avoids us firing an assert that makes EME debugging more painful. I'd like to avoid having this issue in ESR68, which has a long lifetime ahead.
  • User impact if declined: Developers attempting to interact with MediaKeys via console.log, the JS debugger, or similar, will have asserts fire. This is a hindrance to developer workflows.
  • Fix Landed on Version: 70.0a1
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Low risk: single line patch that has been discussed with a domain expert (:mccr8).
  • String or UUID changes made by this patch: None

Beta/Release Uplift Approval Request

  • User impact if declined: Developers attempting to interact with MediaKeys via console.log, the JS debugger, or similar, will have asserts fire. This is a hindrance to developer workflows.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Low risk: single line patch that has been discussed with a domain expert (:mccr8).
  • String changes made/needed: None
Attachment #9086513 - Flags: approval-mozilla-esr68?
Attachment #9086513 - Flags: approval-mozilla-beta?

Comment on attachment 9086513 [details]
Bug 1571081 - Make MediaKeys correctly specify a NS_INTERFACE_MAP_ENTRY for nsISupports. r?mccr8

Simple quality of life improvement for developers. Approved for 69.0b16 and 68.1esr.

Attachment #9086513 - Flags: approval-mozilla-esr68?
Attachment #9086513 - Flags: approval-mozilla-esr68+
Attachment #9086513 - Flags: approval-mozilla-beta?
Attachment #9086513 - Flags: approval-mozilla-beta+
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: