Crash in [@ mozilla::dom::ToJSValue | mozilla::dom::RTCRtpReceiver_Binding::getStats_promiseWrapper]
Categories
(Core :: WebRTC: Signaling, defect)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox-esr91 | --- | unaffected |
| firefox97 | --- | unaffected |
| firefox98 | + | fixed |
| firefox99 | --- | fixed |
People
(Reporter: gsvelto, Assigned: bwc)
References
(Regression)
Details
(Keywords: crash, regression)
Crash Data
Attachments
(2 files)
|
48 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details | Review |
|
48 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details | Review |
Crash report: https://crash-stats.mozilla.org/report/index/e98c1cc9-91bc-483c-a024-91ebd0220205
Reason: EXCEPTION_ACCESS_VIOLATION_READ
Top 4 frames of crashing thread:
0 xul.dll mozilla::dom::ToJSValue dom/bindings/ToJSValue.cpp:64
1 xul.dll mozilla::dom::RTCRtpReceiver_Binding::getStats_promiseWrapper dom/bindings/RTCRtpReceiverBinding.cpp:141
2 xul.dll mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ConvertExceptionsToPromises> dom/bindings/BindingUtils.cpp:3306
3 None @0x0000004ba7d6877e
This appears to be a regression. It only happens on nightly and started with buildid 20220202093701.
|
Assignee | |
Comment 1•2 years ago
|
||
None of these crash reports seem to have a very useful stack. I suppose it is possible that the bug is in the generated binding code, or even BindingUtils, but if the bug is in webrtc code, it is hard to tell where.
|
|
Assignee | |
Comment 2•2 years ago
|
||
Maybe we're hitting this?
We should probably be doing something other than returning a nullptr here. Either this is in a "should never happen" category, in which case a MOZ_CRASH() might be appropriate, or this needs to return a rejected promise. Since this needs fixing anyway, I'll go ahead and create a patch, and see if landing it helps.
|
|
Assignee | |
Updated•2 years ago
|
|
||
Updated•2 years ago
|
|
|
Assignee | |
Comment 3•2 years ago
|
||
|
|
Assignee | |
Comment 4•2 years ago
|
||
|
||
Comment 6•2 years ago
|
||
I'm able to repro bp-3f82b9c1-66cc-4514-ad29-0f1f60220208 with https://jsfiddle.net/jib1/6rzL3asv/16/
JS calling stats on stopped transceivers or even closed peer connections is expected and must not reject. It should succeed, with stats even. See bug 1056433 comment 5.
|
||
Updated•2 years ago
|
|
Reporter | |
Comment 7•2 years ago
•
|
||
Yes, FYI this is a null pointer access and the stack is narrow because it's jumping into JIT'd code so the null pointer is likely coming from there.
|
|
Assignee | |
Comment 8•2 years ago
|
||
(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #6)
I'm able to repro bp-3f82b9c1-66cc-4514-ad29-0f1f60220208 with https://jsfiddle.net/jib1/6rzL3asv/16/
JS calling stats on stopped transceivers or even closed peer connections is expected and must not reject. It should succeed, with stats even. See bug 1056433 comment 5.
Oh, that's fantastic! I'll add a wpt that does the same thing, so we have a test for this bug.
|
|
Assignee | |
Comment 9•2 years ago
|
||
|
|
Assignee | |
Comment 10•2 years ago
|
||
Note that we do not pass this test even with the patch, but at least we don't crash.
|
||
Updated•2 years ago
|
|
|
Assignee | |
Comment 11•2 years ago
|
||
|
||
Updated•2 years ago
|
|
|
Assignee | |
Comment 12•2 years ago
|
||
The new wpt apparently passes sometimes (instead of the expected failure, or the crash that would occur before the bug was fixed):
I'll mark that test case accordingly for now. Here's some retriggers on just that test; maybe the other test-case I added is intermittent too:
https://treeherder.mozilla.org/jobs?repo=try&revision=1c0d224c074208cb869d95a702df2f761de84175
|
|
||
Updated•2 years ago
|
|
||
Comment 13•2 years ago
|
||
Pushed by bcampen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/17eea0669f7f Test case for bug. r=jib https://hg.mozilla.org/integration/autoland/rev/668bce80cfa4 Return a resolved promise instead of a nullptr here. r=jib
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/32806 for changes under testing/web-platform/tests
|
||
Comment 15•2 years ago
|
||
| bugherder | ||
|
|
Assignee | |
Comment 16•2 years ago
|
||
Since this was a pretty frequent crash in the wild, we can probably just wait the weekend to see whether this had the desired effect. Needinfo self to check back next week.
Upstream PR merged by moz-wptsync-bot
|
|
||
Comment 18•2 years ago
|
||
I am not seeing crashes on nightly since the patch landed, the volume of crashes on 98 beta is high with this signature, is that a patch we could uplift? Thanks
|
|
Assignee | |
Comment 19•2 years ago
|
||
Comment on attachment 9262701 [details]
Bug 1753938: Return a resolved promise instead of a nullptr here. r?jib
Beta/Release Uplift Approval Request
- User impact if declined: Somewhat frequent crashes in the field.
- Is this code covered by automated tests?: Yes
- 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): All this does is substitute reasonable, benign behavior for a nullptr crash.
- String changes made/needed: None
|
|
Assignee | |
Updated•2 years ago
|
|
|
Assignee | |
Updated•2 years ago
|
|
|
||
Updated•2 years ago
|
|
|
||
Comment 20•2 years ago
|
||
Comment on attachment 9262701 [details]
Bug 1753938: Return a resolved promise instead of a nullptr here. r?jib
Approved for 98 beta 5, thanks.
|
|
||
Updated•2 years ago
|
|
|
||
Comment 21•2 years ago
|
||
| bugherder uplift | ||
Description
•