1820602 - Remaining crashes on JS_SWEPT_TENURED_PATTERN values in DOM bindings

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Andrew McCreight [:mccr8]]

Bug 1808352 described a situation where we inside DOM bindings and calling a method on what is supposed to be a DOM C++ object, and we end up crashing on JS_SWEPT_TENURED_PATTERN (0x4b). This indicates that the object we are calling a method on is actually a JS GC thing, so we are having some kind of type confusion.

Bug 1808352 cleaned up most of these crashes, but I found 2 crashes that look similar in builds where that is fixed, so I figured I'd file a bug in case there's enough information to fix some further issues. Maybe our fuzzers could also find further issues, now that there's a shell test case in that other bug.

• The first crash: bp-41e3ba83-0c08-401e-a9d2-6957c0230303. The signature is: [@ nsCOMPtr<T>::operator bool | nsINode::HasChildren ]. The DOM bindings method is Node_Binding::hasChildNodes. Looking back over the last three months, there are 60 or so crashes along these lines, mostly with this signature. Of course most of those builds don't have the patch for bug 1808352.

• The second crash: bp-b1385b12-586a-4ba4-9a9c-aa9aa0230302. The signature is [@ nsINode::OwnerDoc ]. The DOM bindings method is Element_Binding::removeAttribute. I see about 130 crashes over the last 3 months with this signature and Element_Binding::removeAttribute in the proto signature. As above, all but one of these crashes are on builds that don't have bug 1808352.

Comment 1

[Andrew McCreight [:mccr8]]

Jan, could you take a look at these few residual crashes? I don't know if you'll be able to figure out anything for these remaining crashes, but I don't know how you figured out something from bug 1808352, so I figured I'd ask. Thanks. I'll also try to see what happens with these kinds of crashes as 111 moves to release.

Comment 2

[Andrew McCreight [:mccr8]]

I don't see any crashes on 102 so I won't mark it affected for now.

Comment 3

[Jan de Mooij [:jandem]]

Good find. These are DOM method calls (instead of getters/setters) and for those we currently guard on the JSClass instead of on the shape. I'll see if we can just change that to a shape guard without affecting performance.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Bug 1820602 - Use shape guard instead of class guard for CallDOMFunction. r?iain!

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1820602 - Add test and comment. r?iain!

Depends on D171836

Comment 6

[Jan de Mooij [:jandem]]

Thanks for filing this one, Andrew. I should have noticed this case being different while working on bug 1808352.

Comment 7

[Andrew McCreight [:mccr8]]

No problem. Thanks for the quick fix!

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 9321564 [details]
Bug 1820602 - Use shape guard instead of class guard for CallDOMFunction. r?iain!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not very easy, but maybe with some work and fuzzing.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: All
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely.
• Is Android affected?: Yes

Comment 9

[Jan de Mooij [:jandem]]

This will probably need a slightly different patch for ESR 102 because it doesn't have the changes from bug 1804253. Shouldn't be hard to fix though.

Comment 10

[Tom Ritter [:tjr] (OOTO until April)]

Comment on attachment 9321564 [details]
Bug 1820602 - Use shape guard instead of class guard for CallDOMFunction. r?iain!

Approved to land and uplift

Comment 11

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/autoland/rev/71001ef878d761ce486458f2c25a1188360d4135

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 9321564 [details]
Bug 1820602 - Use shape guard instead of class guard for CallDOMFunction. r?iain!

Beta/Release Uplift Approval Request

• User impact if declined: Crashes, security bugs.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• 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): Pretty local change in code that should have good test coverage.
• String changes made/needed: N/A
• Is Android affected?: Yes

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Bug 1820602 (ESR102) - Use shape guard instead of class guard for CallDOMFunction.

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 9324166 [details]
Bug 1820602 (ESR102) - Use shape guard instead of class guard for CallDOMFunction.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Crashes, security bugs.
• Fix Landed on Version: 112+
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Use shape guard instead of class guard for CallDOMFunction. r=iain
https://hg.mozilla.org/integration/autoland/rev/71001ef878d761ce486458f2c25a1188360d4135
https://hg.mozilla.org/mozilla-central/rev/71001ef878d7

Comment 16

[Dianna Smith [:diannaS]]

Comment on attachment 9321564 [details]
Bug 1820602 - Use shape guard instead of class guard for CallDOMFunction. r?iain!

Approved for 112.0b6

Comment 17

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fe572ed97f0c

Comment 18

[Dianna Smith [:diannaS]]

Comment on attachment 9324166 [details]
Bug 1820602 (ESR102) - Use shape guard instead of class guard for CallDOMFunction.

Approved for 102.10esr

Comment 19

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/43190ea413e9

Comment 20

[Andrew McCreight [:mccr8]]

I looked through crashes on 112 and higher with the patch on the swept poison value, and I didn't see any crashes in DOM bindings code, so that's good. I'll try to do another check once this patch has been on release for a bit.

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

2 months ago, Tom Ritter [:tjr] placed a reminder on the bug using the whiteboard tag [reminder-test 2023-05-23] .

jandem, please refer to the original comment to better understand the reason for the reminder.

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Add test and comment. r=iain
https://hg.mozilla.org/integration/autoland/rev/ce69715cc73d6d870844c58a5fd1fafc51f60e7c
https://hg.mozilla.org/mozilla-central/rev/ce69715cc73d