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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: 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.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Similar testcases crash at js::ComputeImplicitThis.

Comment 2

[Luke Wagner [:luke]]

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).

Comment 3

[Luke Wagner [:luke]]

Attachment: fix and test

Simple fix: most of the patch is renaming LookupNameForSet, only real change is JSOP_IMPLICITTHIS.

Comment 4

[Alex Keybl [:akeybl]]

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?

Comment 5

[Luke Wagner [:luke]]

Attachment: fix and test

Oops, need the symmetric fix in the JM stub call.  IM doesn't handle IMPLICITTHIS.

Comment 6

[Luke Wagner [:luke]]

(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.

Comment 7

[Jeff Walden [:Waldo]]

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...

Comment 8

[Luke Wagner [:luke]]

(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.

Comment 9

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b5a786ee47b

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8b5a786ee47b

Comment 11

[Alex Keybl [:akeybl]]

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 12

[Luke Wagner [:luke]]

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

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Release drivers, ping re: approval-mozilla-aurora?

Comment 14

[bhavana bajaj [:bajaj]]

Comment on attachment 666754 [details] [diff] [review]
fix and test

 Approving for aurora . Please land before monday oct 8th merge.

Comment 15

[Luke Wagner [:luke]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/28ef082de058

Comment 16

[Manuela Muntean [Away]]

No new crashes appear in Socorro in the last month.

Comment 17

[Manuela Muntean [Away]]

In the last month, no new crashes appear in Socorro.