379482 - Crash [@ js_IsIdentifier] decompiling float setter

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

js> function() { ({ 1.1 setter: 2 }) }
Segmentation fault


Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_INVALID_ADDRESS (0x0001) at 0x3ff19999

Thread 0 Crashed:
0   js 	0x000b4a01 js_IsIdentifier + 235 (jsscan.c:164)
1   js 	0x00094127 Decompile + 38716 (jsopcode.c:3959)
2   js 	0x000954d2 js_DecompileCode + 492 (jsopcode.c:4318)
3   js 	0x00095e8b js_DecompileFunction + 1891 (jsopcode.c:4516)
4   js 	0x0001aa36 JS_DecompileFunction + 84 (jsapi.c:4668)
5   js 	0x000503db js_fun_toString + 494 (jsfun.c:1528)
6   js 	0x0005043f fun_toString + 53 (jsfun.c:1539)
7   js 	0x00059269 js_Invoke + 2945 (jsinterp.c:1332)
8   js 	0x0005968c js_InternalInvoke + 309 (jsinterp.c:1426)
9   js 	0x000854f0 js_TryMethod + 346 (jsobj.c:4633)
10  js 	0x00083f24 js_DefaultValue + 143 (jsobj.c:3924)
11  js 	0x000c7550 js_ValueToString + 98 (jsstr.c:2656)
12  js 	0x00008bc3 JS_ValueToString + 24 (jsapi.c:543)
13  js 	0x00002197 Process + 949 (js.c:270)
14  js 	0x00002b7b ProcessArgs + 2045 (js.c:519)
15  js 	0x00007a40 main + 612 (js.c:3237)
16  js 	0x00001c86 _start + 216
17  js 	0x00001bad start + 41

Comment 1

[Brian Crowder]

This bug did not exist before the code in bug 356083 landed?

Comment 2

[Brian Crowder]

Attachment: not all atoms are strings, crowder

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 263510 [details] [diff] [review]
not all atoms are strings, crowder

Aren't keywords allowed now in property id contexts? Asking igor for second review.

/be

Comment 4

[Igor Bukanov]

(In reply to comment #3)
> (From update of attachment 263510 [details] [diff] [review])
> Aren't keywords allowed now in property id contexts? Asking igor for second
> review.

The are not allowed after get/set:

js> var x = { get in1() { return 10; } } 
js> var x = { get in() { return 10; } }  
typein:4: SyntaxError: missing : after property id:
typein:4: var x = { get in() { return 10; } }
typein:4: ..............^

I consider this is a bug since keywords is allowed as function names and AFAICS  supporting get keyword should not be a pronlem.

Comment 5

[Brian Crowder]

So instead of using IS_KEYWORD, I should check specifically against "this"?  I was worried about missing cases by doing that, but I agree with Igor (I think) that "get in" should work.  The keyword fix is related to bug 356083, btw.

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #5)
> So instead of using IS_KEYWORD, I should check specifically against "this"?

No (why would 'this' be the only case?). The point is that you can avoid the old setter: syntax and use set this(v) { ... } instead of "this" setter: function (v) {...}, and likewise for any keyword (not just for 'this').

> I
> was worried about missing cases by doing that, but I agree with Igor (I think)
> that "get in" should work.  The keyword fix is related to bug 356083, btw.

The keyword fix I'm looking for is the bug not yet on file (right, Igor?) about get in() {...} within an object initialiser not being legal because 'in' is a keyword.

I think we want that bug to block this one, and this one should then allow keywords in the new-style getters and setters in object initialisers. Igor, what do you think?

/be

Comment 7

[Brian Crowder]

I misunderstood this comment, then:  https://bugzilla.mozilla.org/show_bug.cgi?id=356083#c6 ...  I'll file the new bug and propose a patch, if we're all agreed?

I wrote the IS_KEYWORD check in that bug (not this one) because of the 'this' problem.  So any new bug should probably block that one and not this one.

Comment 8

[Brendan Eich [:brendan]]

Sorry, my comment was written without benefit of Igor's keywords legal in function names patch -- or if that patch had landed by then, we didn't make the connection to get foo() {...} / set foo(x) {...} style getters and setters.

It's safe to bounce keyword identifiers to the old syntax, but not nice in light of ES4. Better to allow keywords in new-style getters and setters, and patch this bug to force old-style only for non-identifier property names.

/be

Comment 9

[Igor Bukanov]

I filed bug 379566 about keywords after get/set and made it blocking this one. 

Comment 10

[Brian Crowder]

jsopcode.c: 3.327

Comment 11

[Jesse Ruderman]

Still crashes for me.

function() { ({ 1.1 setter: 2 }) }

Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_PROTECTION_FAILURE (0x0002) at 0x00000008

Thread 0 Crashed:
0   js 	0x00094ad7 Decompile + 38814 (jsopcode.c:3987)
1   js 	0x00095eb0 js_DecompileCode + 492 (jsopcode.c:4349)
2   js 	0x00096869 js_DecompileFunction + 1891 (jsopcode.c:4547)
3   js 	0x0001b096 JS_DecompileFunction + 84 (jsapi.c:4668)
4   js 	0x00050b97 js_fun_toString + 494 (jsfun.c:1528)
5   js 	0x00050bfb fun_toString + 53 (jsfun.c:1539)
6   js 	0x00059a49 js_Invoke + 2945 (jsinterp.c:1332)
7   js 	0x00059e6c js_InternalInvoke + 309 (jsinterp.c:1426)
8   js 	0x00085e58 js_TryMethod + 346 (jsobj.c:4633)
9   js 	0x0008488c js_DefaultValue + 143 (jsobj.c:3924)
10  js 	0x000c7f48 js_ValueToString + 98 (jsstr.c:2656)
11  js 	0x00009223 JS_ValueToString + 24 (jsapi.c:543)
12  js 	0x000027f7 Process + 949 (js.c:270)
13  js 	0x000031db ProcessArgs + 2045 (js.c:519)
14  js 	0x000080a0 main + 612 (js.c:3237)
15  js 	0x000022e6 _start + 216
16  js 	0x0000220d start + 41

Comment 12

[Brian Crowder]

Attachment: null check

Not sure how I overlooked this.

Comment 13

[Brian Crowder]

It's interesting to me, btw, that the object literal here doesn't get optimized away, since it is never used and never returned by the function.  SHOULD it be?

Comment 14

[Jesse Ruderman]

I can't be optimized away as a useless expression-statement, because if you redefined Object or put some setters on Object.prototype, you'd expect your code to be called.  (But see bug 376957 comment 0, especially proposal B.)

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 264451 [details] [diff] [review]
null check

It's easy to miss this -- it's a result of code that flows from the goto label (namely from the JSOP_INITELEM code).

Comment 16

[Brian Crowder]

jsopcode.c: 3.242

Comment 17

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_7/extensions/regress-379482.js,v  <--  regress-379482.js
initial revision: 1.1

Comment 18

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 linux/mac*/windows.