346773 - Crash with misplaced braces in function

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jeff Walden [:Waldo]]

js> var it = {foo:"5"};
js> it.__iterator__ =
    function(valsOnly)
    {
      var gen =
        function()
        {
          for (var i = 0; i < keys.length; i++)
          {
            if (valsOnly)
              yield vals[i];
            else
              yield [keys[i], vals[i]];
          }
      return gen();
./js: line 2: 16626 Segmentation fault      /home/jwalden/moz/trees/js/mozilla/js/src/Linux_All_DBG.OBJ/js

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix copy-paste error

Obvious copy/paste error.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 231531 [details] [diff] [review]
Fix copy-paste error

Oh, for type-safe format strings!

/be

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Checked in to trunk.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Seems like you could have some DEBUG-only code (along with the existing assertions in JS_NewRuntime) that has assertions to check for this problem.

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 231531 [details] [diff] [review]
Fix copy-paste error

a=drivers, please land this on the MOZILLA_1_8_BRANCH.

Comment 6

[Brendan Eich [:brendan]]

Attachment: how about this?

(In reply to comment #4)
> Seems like you could have some DEBUG-only code (along with the existing
> assertions in JS_NewRuntime) that has assertions to check for this problem.

This is a DEBUG-only change, so dbaron feel free to approve for 1.8.1 as well as sr+ -- thanks.

/be

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 231544 [details] [diff] [review]
how about this?

>+         * error names enumerated in jscntxt.c.  It's not a compiletime check,

I'm skeptical of compiletime being one word.  Perhaps hyphenate?

I wanted to use existing style of the JS engine to point out some nits, but apparently there is little consistency in js/src/ for choosing between prefix and postfix increment or decrement when it doesn't matter.  (I prefer prefix.)

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> (From update of attachment 231544 [details] [diff] [review] [edit])
> >+         * error names enumerated in jscntxt.c.  It's not a compiletime check,
> 
> I'm skeptical of compiletime being one word.  Perhaps hyphenate?

I merely rewrapped after indenting, vim did the work.  This is mccabe's comment, IIRC.  Fixed.

> I wanted to use existing style of the JS engine to point out some nits, but
> apparently there is little consistency in js/src/ for choosing between prefix
> and postfix increment or decrement when it doesn't matter.  (I prefer prefix.)

In C code, for expression-statements or expressions whose value is discarded, it doesn't matter.  for loops use i++ in the third part of the head, generally; we are moving toward ++i; for expression statements, just to match Mozilla C++ code style.  Was there any other kind of nit you saw?

/be

Comment 9

[Brendan Eich [:brendan]]

Fixed on trunk and branch.

/be

Comment 10

[Bob Clary [:bc] (inactive)]

Checking in regress-346773.js;
/cvsroot/mozilla/js/tests/js1_7/geniter/regress-346773.js,v  <--  regress-346773.js
initial revision: 1.1

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 231544 [details] [diff] [review]
how about this?

Yes, please.

Comment 12

[Bob Clary [:bc] (inactive)]

test case returns error: anonymous generator function returns a value
Is that a bug?

crash: verified fixed 1.8, 1.9 windows/mac(ppc|tel)/linux

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> test case returns error: anonymous generator function returns a value
> Is that a bug?

In the testcase?  Yes.  You cannot return e; in a function containing yield expressions.  You can fall off the end or return; (equivalently), which will throw StopIteration.

/be

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #12)
> test case returns error: anonymous generator function returns a value
> Is that a bug?

To be clear, the crash was caused by attempting to report *this* error. The mismatched braces didn't actually play into the testcase.

Comment 15

[Bob Clary [:bc] (inactive)]

Checking in regress-346773.js;
/cvsroot/mozilla/js/tests/js1_7/geniter/regress-346773.js,v  <--  regress-346773.js
new revision: 1.2; previous revision: 1.1

confirmed this version of the test still crashes in 07/31 builds. the return gen() was required for the crash.