Closed Bug 795574 Opened 12 years ago Closed 12 years ago

Crash [@ js::GetObjectClass] or [@ js::ComputeImplicitThis]

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla18
Tracking Status
firefox15 --- unaffected
firefox16 --- unaffected
firefox17 + verified
firefox18 + verified
firefox-esr10 --- unaffected

People

(Reporter: gkw, Assigned: luke)

References

Details

(Keywords: crash, regression, testcase, Whiteboard: [jsbugmon:update])

Crash Data

Attachments

(2 files, 1 obsolete file)

Attached file stack
Function("\ __defineGetter__(\"x\", function() {\ delete this[\"x\"];\ });\ x()\ ")() crashes js debug and opt shell on m-c changeset c09a0c022b2e without any CLI arguments at js::GetObjectClass autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: 102943:57c1c330e85f user: Luke Wagner date: Fri Aug 17 18:09:43 2012 -0700 summary: Bug 774915 - don't use the property cache for dynamic name lookup (r=bhackett) Inspection of the registers shows to likely be a null deref (%rax and %rcx in %pc are null or near-null), but setting s-s just to be safe.
Similar testcases crash at js::ComputeImplicitThis.
Crash Signature: [@ js::GetObjectClass] → [@ js::GetObjectClass] [@ js::ComputeImplicitThis]
Summary: Crash [@ js::GetObjectClass] → Crash [@ js::GetObjectClass] or [@ js::ComputeImplicitThis]
Crash Signature: [@ js::GetObjectClass] [@ js::ComputeImplicitThis] → [@ js::GetObjectClass] [@ js::ComputeImplicitThis]
Ahh, IMPLICITTHIS can, as this example demonstrates, miss, which returns a null 'scope' hence the (safe) NULL deref. IMPLICITTHIS should use the 'global'-defaulting LookupNameForSet variant (which now needs to be renamed).
Group: core-security
Crash Signature: [@ js::GetObjectClass] [@ js::ComputeImplicitThis] → [@ js::GetObjectClass] [@ js::ComputeImplicitThis]
Attached patch fix and test (obsolete) — Splinter Review
Simple fix: most of the patch is renaming LookupNameForSet, only real change is JSOP_IMPLICITTHIS.
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #666741 - Flags: review?(jwalden+bmo)
We don't track all regressions for a specific release. This isn't a high volume crash or a security issue, so what's the user impact here?
Attached patch fix and testSplinter Review
Oops, need the symmetric fix in the JM stub call. IM doesn't handle IMPLICITTHIS.
Attachment #666741 - Attachment is obsolete: true
Attachment #666741 - Flags: review?(jwalden+bmo)
Attachment #666754 - Flags: review?(jwalden+bmo)
(In reply to Alex Keybl [:akeybl] from comment #4) > We don't track all regressions for a specific release. This isn't a high > volume crash or a security issue, so what's the user impact here? If a site triggers the condition (which seems relatively rare, but the web is vast), we'll get a null deref. It's hard to tell what the actual crash volume is; ComputeImplicitThis is an inline function which means that crashes will likely show up as js::Interpret in release builds.
Crash Signature: [@ js::GetObjectClass] [@ js::ComputeImplicitThis] → [@ js::GetObjectClass] [@ js::ComputeImplicitThis]
Comment on attachment 666754 [details] [diff] [review] fix and test Review of attachment 666754 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/basic/testImplicitThisMiss.js @@ +1,3 @@ > +// |jit-test| error:TypeError > +var f = Function("\ > + __defineGetter__(\"x\", function() {\ Object.defineProperty(this, "x", { configurable: true, get: function() { ... } }); @@ +1,4 @@ > +// |jit-test| error:TypeError > +var f = Function("\ > + __defineGetter__(\"x\", function() {\ > + delete this[\"x\"];\ Use ' for readability in these lines? Or maybe convert this whole thing into a function expression, not a runtime-compiled function? Would read a whole lot better that way...
Attachment #666754 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7) > Or maybe convert this whole thing into a function expression, Function is necessary to generate JSOP_IMPLICITTHIS.
Crash Signature: [@ js::GetObjectClass] [@ js::ComputeImplicitThis] → [@ js::GetObjectClass] [@ js::ComputeImplicitThis]
Crash Signature: [@ js::GetObjectClass] [@ js::ComputeImplicitThis] → [@ js::GetObjectClass] [@ js::ComputeImplicitThis]
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Crash Signature: [@ js::GetObjectClass] [@ js::ComputeImplicitThis] → [@ js::GetObjectClass] [@ js::ComputeImplicitThis]
Tracking for FF17, but if this doesn't get fixed while 17 is on Aurora and has a non-zero risk fix, we'll likely untrack due to lack of data.
Comment on attachment 666754 [details] [diff] [review] fix and test [Approval Request Comment] Bug caused by (feature/regressing bug #): 774915 User impact if declined: null deref crash Testing completed (on m-c, etc.): m-c Risk to taking this patch (and alternatives if risky): seems pretty low, just restoring accidentally-changed previous behavior in a rare op
Attachment #666754 - Flags: approval-mozilla-aurora?
Release drivers, ping re: approval-mozilla-aurora?
Comment on attachment 666754 [details] [diff] [review] fix and test Approving for aurora . Please land before monday oct 8th merge.
Attachment #666754 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: verifyme
No new crashes appear in Socorro in the last month.
In the last month, no new crashes appear in Socorro.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: