Attempting to log MediaKeys to console triggers assert
Categories
(Core :: Audio/Video: Playback, defect, P2)
Tracking
()
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)
722 bytes,
text/html
|
Details | |
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-esr68+
|
Details | Review |
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.
Assignee | ||
Comment 1•5 years ago
|
||
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 | ||
Comment 2•5 years ago
|
||
Backing out the last changeset from 1522547 alleviates the issue. Looks like my changing of the macros in that patch broke something.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 3•5 years ago
|
||
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.
Assignee | ||
Comment 4•5 years ago
|
||
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
Comment 6•5 years ago
|
||
bugherder |
Comment 7•5 years ago
|
||
What's the user impact of this bug? Does this need to be nominated for uplift or can it ride the trains?
Assignee | ||
Comment 8•5 years ago
|
||
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?
Comment 9•5 years ago
|
||
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.
Assignee | ||
Comment 10•5 years ago
|
||
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
Comment 11•5 years ago
|
||
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.
Comment 12•5 years ago
|
||
bugherder uplift |
Comment 13•5 years ago
|
||
bugherder uplift |
Updated•2 years ago
|
Description
•