Last Comment Bug 356378 - "invalid getter usage" or assertion failure with "var x; x getter= function () { };"
: "invalid getter usage" or assertion failure with "var x; x getter= function (...
Status: VERIFIED FIXED
[sg:critical?]
: assertion, crash, testcase, verified1.8.1.15, verified1.8.1.16
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: P2 critical (vote)
: mozilla1.9
Assigned To: Igor Bukanov
:
Mentors:
Depends on:
Blocks: jsfunfuzz 435497
  Show dependency treegraph
 
Reported: 2006-10-11 23:49 PDT by Jesse Ruderman
Modified: 2008-08-16 04:00 PDT (History)
19 users (show)
mbeltzner: blocking1.9+
dveditz: blocking1.8.1.15+
samuel.sidler+old: blocking1.8.1.16+
dveditz: wanted1.8.1.x+
bob: in‑testsuite+
bob: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (2.56 KB, patch)
2008-03-23 06:53 PDT, Igor Bukanov
brendan: review+
mbeltzner: approval1.9b5+
Details | Diff | Splinter Review
js1_5/extensions/regress-356378.js (2.47 KB, text/plain)
2008-03-29 14:41 PDT, Bob Clary [:bc:]
no flags Details
fix for 18 branch (2.82 KB, patch)
2008-05-28 14:54 PDT, Igor Bukanov
brendan: review+
dveditz: approval1.8.1.15+
Details | Diff | Splinter Review
diff between 1.9 and 1.8 versions of the fix (2.39 KB, text/plain)
2008-05-28 14:57 PDT, Igor Bukanov
no flags Details
one-liner fix for 1.8.1 regression (1.24 KB, patch)
2008-06-12 12:26 PDT, Igor Bukanov
igor: review+
dveditz: approval1.8.1.15+
dveditz: approval1.8.1.16+
Details | Diff | Splinter Review
fix for 18 branch with the extra regression fix included (2.84 KB, patch)
2008-06-12 12:47 PDT, Igor Bukanov
igor: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2006-10-11 23:49:42 PDT
js> (function() { var x; x getter= function () { }; })()

Opt: 
  typein:1: SyntaxError: invalid getter usage
  (Why?)

Debug: 
  Assertion failure: 0, at jsinterp.c:5128
Comment 1 Blake Kaplan (:mrbkap) 2006-10-23 23:57:59 PDT
JSOP_GETTER and JSOP_SETTER don't handle the optimized forms (getarg, getvar, getlocal, etc). My first inclination is to deoptimize variables (introduced by let or var) that have getters stuck on them, and treat them as name lookups from the point of the getter onwards. This would involve grabbing the scope chain for let variables (and somehow making sure that we don't accidentally bind the name to the slot again in that scope).

The "invalid getter usage" is because we never set rval in the JSOP_GETTER case, and it happens to contain a primitive value when we run that code.
Comment 2 Jesse Ruderman 2007-08-01 20:01:00 PDT
This assertion failure is one of the more frequent for jsfunfuzz to hit nowadays.
Comment 3 Aiko 2007-08-21 02:42:57 PDT
The procedure in comment 1 sounds like a lot of work just to make the fuzzer happy about a language construct that was deprecated over seven years ago. I'd like to point out that so far there hasn't been a single bug filed about the fact that specifying getters or setters for local variables or function arguments doesn't work at all. Wouldn't the better way to fix this bug be to finally remove support for [g|s]etter= , or even [g|s]etter altogether?
Comment 4 Mike Beltzner [:beltzner, not reading bugmail] 2008-03-02 22:08:44 PST
Brendan: blocking1.9 with a target milestone of beta4 means that this blocks shipment of beta4. If you don't think that's necessary, please change the TM to "mozilla1.9".
Comment 5 Igor Bukanov 2008-03-03 12:24:30 PST
Brendan: I can take this to create a patch that throws a syntax error at the compile time for var getter and friends.
Comment 6 Brendan Eich [:brendan] 2008-03-03 13:15:29 PST
Igor, thanks.

/be
Comment 7 Damon Sicore (:damons) 2008-03-13 11:50:03 PDT
Does this bug require a beta vector or is it safe to ship in an RC?  Just want to make sure it's on the right list.
Comment 8 Jesse Ruderman 2008-03-18 11:47:57 PDT
jsfunfuzz hits this about once an hour, but it doesn't really need to be a Gecko 1.9 blocker.
Comment 9 Mike Schroepfer 2008-03-18 11:49:40 PDT
Based on comment 8 taking off the list...
Comment 10 Igor Bukanov 2008-03-23 06:53:33 PDT
Created attachment 311242 [details] [diff] [review]
v1

The patch changes the emitter to throw an exception when it detects unsupported getter/setter usage.
Comment 11 Igor Bukanov 2008-03-23 07:01:03 PDT
Marking the bug as security sensitive as the bug effectively means misinterpreting the values on the stack. On 1.8 branch the bug is also a GC hazard.  
Comment 12 Igor Bukanov 2008-03-23 07:02:26 PDT
Nominating for 1.9 as this is security hazard.
Comment 13 Brendan Eich [:brendan] 2008-03-24 01:12:00 PDT
Comment on attachment 311242 [details] [diff] [review]
v1

Thanks (especially for that second hunk -- not sure what I was thinking there!).

/be
Comment 14 Mike Beltzner [:beltzner, not reading bugmail] 2008-03-24 05:57:34 PDT
Comment on attachment 311242 [details] [diff] [review]
v1

a=beltzner
Comment 16 Igor Bukanov 2008-03-24 06:41:44 PDT
Nominating for 1.8 branch
Comment 17 Bob Clary [:bc:] 2008-03-29 14:41:28 PDT
Created attachment 312526 [details]
js1_5/extensions/regress-356378.js
Comment 18 Bob Clary [:bc:] 2008-04-17 14:19:56 PDT
v 1.9.0
Comment 19 Igor Bukanov 2008-05-28 14:54:55 PDT
Created attachment 322858 [details] [diff] [review]
fix for 18 branch

The patch is straightforward backport to 1.8 branch
Comment 20 Igor Bukanov 2008-05-28 14:57:17 PDT
Created attachment 322860 [details]
diff between 1.9 and 1.8 versions of the fix
Comment 21 Brendan Eich [:brendan] 2008-05-29 14:09:31 PDT
Comment on attachment 322858 [details] [diff] [review]
fix for 18 branch

Thanks, looks good.

/be
Comment 22 Daniel Veditz [:dveditz] 2008-05-30 11:27:50 PDT
Comment on attachment 322858 [details] [diff] [review]
fix for 18 branch

Approved for 1.8.1.15, a=dveditz for release-drivers
Comment 23 Igor Bukanov 2008-06-06 14:42:26 PDT
I checked in the patch from the comment 19 to the MOZILLA_1_8_BRANCH:

Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.128.2.76; previous revision: 3.128.2.75
done
Comment 24 Bob Clary [:bc:] 2008-06-11 01:47:08 PDT
On the 1.8.1 branch the original assert reported in comment 0 is gone but there is a new one:

on  2008-06-05 the assert was

Assertion failure: op2 == JSOP_INITELEM, at jsinterp.c:5132

on 2008-06-08 the assert changed to 

Assertion failure: !ts || ts->linebuf.limit < ts->linebuf.base + JS_LINE_LIMIT, at jsscan.c:572

removed fixed1.8.1.15 or file a different bug?


Comment 25 Igor Bukanov 2008-06-12 09:22:50 PDT
(In reply to comment #24)
> removed fixed1.8.1.15 or file a different bug?

This is the result of the bad backport: I did not see the bug with the standalone testcase from the commnet 1, only the full test case exposes it. The bug is that I have used:

                js_ReportCompileErrorNumber(cx,
                                            pn2, JSREPORT_ERROR,
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                            JSMSG_BAD_GETTER_OR_SETTER,
                                            (op == JSOP_GETTER)
                                            ? js_getter_str
                                            : js_setter_str);


when it should be:

                js_ReportCompileErrorNumber(cx,
                                            pn2, JSREPORT_PN | JSREPORT_ERROR,
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                            JSMSG_BAD_GETTER_OR_SETTER,
                                            (op == JSOP_GETTER)
                                            ? js_getter_str
                                            : js_setter_str);

The result of this regression is the reinterpretation of JSParseNode as JSTokenStream. If it is not too late I would like to back out the patch and check in the proper fix.
Comment 26 Samuel Sidler (old account; do not CC) 2008-06-12 12:21:41 PDT
We're going to take this on the relbranch for 1.8.1.15. Details to follow. Marking this as blocking1.8.1.16 so we make sure we get it in both places.
Comment 27 Igor Bukanov 2008-06-12 12:26:15 PDT
Created attachment 324843 [details] [diff] [review]
one-liner fix for 1.8.1 regression

This is trivial correctness fix for the regression. The current plan is to land it to 1.8.1.15 release branch, then back out the broken fix from 1.8.1, then create a new patch with the fix included, and then check it in to 1.8.1 branch.
Comment 28 Daniel Veditz [:dveditz] 2008-06-12 12:34:02 PDT
Comment on attachment 324843 [details] [diff] [review]
one-liner fix for 1.8.1 regression

approved for 1.8.1.15, a=dveditz for release-drivers

Since we've already branched for release this will need to land on BOTH the MOZILLA_1_8_BRANCH and GECKO181_20080612_RELBRANCH. when landed please add both fixed1.8.1.15 and fixed1.8.1.16 so we can track and QA both builds.
Comment 29 Igor Bukanov 2008-06-12 12:39:17 PDT
I checked in the one-liner patch from the comment 27 to GECKO181_20080612_RELBRANCH:

Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.128.2.77.2.1; previous revision: 3.128.2.77
done
Comment 30 Igor Bukanov 2008-06-12 12:44:58 PDT
I backed out the patch from the comment 19 from MOZILLA_1_8_BRANCH:

Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.128.2.78; previous revision: 3.128.2.77
done
Comment 31 Igor Bukanov 2008-06-12 12:47:08 PDT
Created attachment 324847 [details] [diff] [review]
fix for 18 branch with the extra regression fix included

Here comes a patch that I should create in the first place for MOZILLA_1_8_BRANCH
Comment 32 Igor Bukanov 2008-06-12 12:51:08 PDT
I re-landed the update patch from the comment 31 to MOZILLA_1_8_BRANCH:

Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.128.2.79; previous revision: 3.128.2.78
done
Comment 33 Bob Clary [:bc:] 2008-06-12 14:42:45 PDT
js1_5/extensions/regress-356378.js passed on GECKO181_20080612_RELBRANCH and MOZILLA_1_8_BRANCH on linux for shell|browser, opt|debug

verified1.8.1.15, verified1.8.1.16

Comment 34 neil@parkwaycc.co.uk 2008-06-21 09:51:20 PDT
Comment on attachment 324847 [details] [diff] [review]
fix for 18 branch with the extra regression fix included

>Index: jsemit.c

>+            // FALL THROUGH
Someone pointed out in one of the newsgroups that this isn't valid C.
Comment 35 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-06-22 10:10:33 PDT
What's invalid about it?  C permits inter-case fall-through, and always has.  I'm pretty sure they didn't make it illegal in C99, since huge amounts of code would break.  (C99 also introduces C++-style //-to-EOL comments, if that's what you're referring to.)
Comment 36 Brian Crowder 2008-06-22 13:18:44 PDT
It would, though, be the only use of a C99/C++-style comment anywhere in spidermonkey.  There must have once been a reason for this prevailing style?  Perhaps we should adhere to it, at least for non-mozilla-central-only patches?
Comment 37 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-06-22 13:33:34 PDT
Yeah, there was once a goal of compiling with C compilers that didn't support that comment syntax; I don't think that goal remains (nor could I easily name such a compiler with which we currently compile otherwise).  Still just guessing what Neil was referring to, of course. :)
Comment 38 Bob Clary [:bc:] 2008-07-03 07:35:48 PDT
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-356378.js,v  <--  regress-356378.js
initial revision: 1.1

changeset:   15652:59d04267975b

Comment 39 Uli Link (:ul-mcamafia) 2008-08-16 04:00:42 PDT
(In reply to comment #35)
> What's invalid about it?  C permits inter-case fall-through, and always has. 
> I'm pretty sure they didn't make it illegal in C99, since huge amounts of code
> would break.  (C99 also introduces C++-style //-to-EOL comments, if that's what
> you're referring to.)

MOZILLA_1_8_BRANCH is still supported on pre-C99 compiler as VisualAge 5.0.2+fixes on AIX 4.3.3 or older HP-UX compilers.
see bug 450865
GCC and MSVC are accepting C++ style for decades, but that's not ANSI C89 conforming.


Note You need to log in before you can comment on or make changes to this bug.