Closed
Bug 795574
Opened 12 years ago
Closed 12 years ago
Crash [@ js::GetObjectClass] or [@ js::ComputeImplicitThis]
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
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)
6.29 KB,
text/plain
|
Details | |
6.64 KB,
patch
|
Waldo
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
![]() |
Reporter | |
Comment 1•12 years ago
|
||
Similar testcases crash at js::ComputeImplicitThis.
Crash Signature: [@ js::GetObjectClass] → [@ js::GetObjectClass]
[@ js::ComputeImplicitThis]
Summary: Crash [@ js::GetObjectClass] → Crash [@ js::GetObjectClass] or [@ js::ComputeImplicitThis]
Updated•12 years ago
|
Crash Signature: [@ js::GetObjectClass]
[@ js::ComputeImplicitThis] → [@ js::GetObjectClass]
[@ js::ComputeImplicitThis]
![]() |
Assignee | |
Comment 2•12 years ago
|
||
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]
![]() |
Assignee | |
Comment 3•12 years ago
|
||
Simple fix: most of the patch is renaming LookupNameForSet, only real change is JSOP_IMPLICITTHIS.
Comment 4•12 years ago
|
||
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?
![]() |
Assignee | |
Comment 5•12 years ago
|
||
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)
![]() |
Assignee | |
Comment 6•12 years ago
|
||
(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.
Updated•12 years ago
|
Crash Signature: [@ js::GetObjectClass]
[@ js::ComputeImplicitThis] → [@ js::GetObjectClass]
[@ js::ComputeImplicitThis]
Comment 7•12 years ago
|
||
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+
![]() |
Assignee | |
Comment 8•12 years ago
|
||
(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]
![]() |
Assignee | |
Comment 9•12 years ago
|
||
Updated•12 years ago
|
Crash Signature: [@ js::GetObjectClass]
[@ js::ComputeImplicitThis] → [@ js::GetObjectClass]
[@ js::ComputeImplicitThis]
Comment 10•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Updated•12 years ago
|
Crash Signature: [@ js::GetObjectClass]
[@ js::ComputeImplicitThis] → [@ js::GetObjectClass]
[@ js::ComputeImplicitThis]
Comment 11•12 years ago
|
||
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.
![]() |
Assignee | |
Comment 12•12 years ago
|
||
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?
![]() |
Reporter | |
Comment 13•12 years ago
|
||
Release drivers, ping re: approval-mozilla-aurora?
Comment 14•12 years ago
|
||
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+
![]() |
Assignee | |
Comment 15•12 years ago
|
||
Comment 16•12 years ago
|
||
No new crashes appear in Socorro in the last month.
Comment 17•12 years ago
|
||
In the last month, no new crashes appear in Socorro.
You need to log in
before you can comment on or make changes to this bug.
Description
•