Crash in js::GetOwnPropertyDescriptor

RESOLVED FIXED in Firefox 57

Status

()

defect
P1
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bc, Assigned: jorendorff)

Tracking

({crash})

58 Branch
mozilla58
Unspecified
Linux
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 wontfix, firefox57 fixed, firefox58 fixed)

Details

(crash signature, )

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

2 years ago
This bug was filed from the Socorro interface and is 
report bp-135340f1-b601-4d4a-98f9-e68320171010.
=============================================================

This crash occurred after the session had been active for a number of hours. The url in the report is https://www.samsung.com/us/mobile/phones/galaxy-s/s/galaxy_s-galaxy_s8/_/n-10+11+hv1rp+zq1xa+troyz/ but I have had that url loaded into a tab for several days now so I don't know how useful it really is.

Similar crash in fixed bug 1234147
Reporter

Comment 1

2 years ago
Happened again after sitting idle: bp-0bf49dcc-fc1b-4a71-845d-721860171011
Jason is this related to the work you and Bz are doing on xray wrappers?
Flags: needinfo?(jorendorff)
Priority: -- → P1
Assignee

Comment 3

2 years ago
Looking to see if the stack is accurate, and if so, whether `this` is null or `this->group_` is null. (Both seem "impossible".)
Assignee

Updated

2 years ago
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Assignee

Comment 5

2 years ago
`this` is null.

This happens if `XrayTraits::singleton.ensureHolder(cx, wrapper)` returns null on failure. We don't check. That only happens on OOM, but I guess it's possible. :-|

bz, while you're looking at this code...

     RootedObject target(cx, XrayTraits::getTargetObject(wrapper));

Why is it `XrayTraits` and not `Traits` there?
Flags: needinfo?(jorendorff)
Assignee

Updated

2 years ago
Flags: needinfo?(bzbarsky)
> Why is it `XrayTraits` and not `Traits` there?

I can't see a good reason, honestly.  I also can't see a good reason for things like singleton.getTargetObject in several places in the file, given that it's a static method...
Flags: needinfo?(bzbarsky)
Comment on attachment 8919334 [details] [diff] [review]
Crash in js::GetOwnPropertyDescriptor

r=me
Attachment #8919334 - Flags: review?(bzbarsky) → review+

Comment 9

2 years ago
Pushed by jorendorff@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0181d804e8bf
Crash in js::GetOwnPropertyDescriptor. r=bz
Assignee

Comment 10

2 years ago
Attachment #8919768 - Flags: review?(mrbkap)
Assignee

Updated

2 years ago
Attachment #8919334 - Attachment is obsolete: true
Assignee

Comment 11

2 years ago
Comment on attachment 8919768 [details] [diff] [review]
Crash in js::GetOwnPropertyDescriptor

sorry, misfiled patch
Attachment #8919768 - Attachment is obsolete: true
Attachment #8919768 - Flags: review?(mrbkap)
Assignee

Updated

2 years ago
Attachment #8919334 - Attachment is obsolete: false

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0181d804e8bf
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Not a hugely-frequent crash, but it's just an added null check. Please nominate this for Beta approval.
Flags: needinfo?(jorendorff)
Boris, do you think we should nominate this for Beta approval? Running low on time to do so if the answer is yes.
Flags: needinfo?(bzbarsky)
Comment on attachment 8919334 [details] [diff] [review]
Crash in js::GetOwnPropertyDescriptor

Approval Request Comment
[Feature/Bug causing the regression]: Been around for a while.
[User impact if declined]: Certain amount of null-deref crashes.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: No.
[Needs manual test from QE? If yes, steps to reproduce]: We don't have good
   steps to reproduce.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It's just adding a check for a failure
   case in which we would crash and propagating the error out.
[String changes made/needed]: None.
Flags: needinfo?(bzbarsky)
Attachment #8919334 - Flags: approval-mozilla-beta?
Comment on attachment 8919334 [details] [diff] [review]
Crash in js::GetOwnPropertyDescriptor

Low-risk crash fix, let's take this for beta 12.
Attachment #8919334 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(jorendorff)
You need to log in before you can comment on or make changes to this bug.