Closed Bug 1407414 Opened 3 years ago Closed 3 years ago
Crash in js::Get
Own Property Descriptor
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
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?
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".)
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
`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?
> 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...
Comment on attachment 8919334 [details] [diff] [review] Crash in js::GetOwnPropertyDescriptor r=me
Attachment #8919334 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/0181d804e8bf9babc3e91f4fcd7d2cf6491372ab Bug 1407414 - Crash in js::GetOwnPropertyDescriptor. r=bz
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/0181d804e8bf Crash in js::GetOwnPropertyDescriptor. r=bz
Attachment #8919334 - Attachment is obsolete: true
Comment on attachment 8919768 [details] [diff] [review] Crash in js::GetOwnPropertyDescriptor sorry, misfiled patch
Attachment #8919334 - Attachment is obsolete: false
Not a hugely-frequent crash, but it's just an added null check. Please nominate this for Beta approval.
Boris, do you think we should nominate this for Beta approval? Running low on time to do so if the answer is yes.
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.
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+
You need to log in before you can comment on or make changes to this bug.