Closed Bug 1407414 Opened 3 years ago Closed 3 years ago

Crash in js::GetOwnPropertyDescriptor


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

58 Branch



Tracking Status
firefox-esr52 --- unaffected
firefox56 --- wontfix
firefox57 --- fixed
firefox58 --- fixed


(Reporter: bc, Assigned: jorendorff)




(Keywords: crash)

Crash Data


(1 file, 1 obsolete file)

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 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
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
Looking to see if the stack is accurate, and if so, whether `this` is null or `this->group_` is null. (Both seem "impossible".)
Attachment #8919334 - Flags: review?(bzbarsky)
Assignee: nobody → jorendorff
`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)
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

Attachment #8919334 - Flags: review?(bzbarsky) → review+
Pushed by
Crash in js::GetOwnPropertyDescriptor. r=bz
Attachment #8919768 - Flags: review?(mrbkap)
Attachment #8919334 - Attachment is obsolete: true
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)
Attachment #8919334 - Attachment is obsolete: false
Closed: 3 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.