437288 - [1.8 branch] Hang involving a for loop turning into a while loop

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

(function() { const x = 1; for (x in null); })()

This hangs js shell 1.8 branch. Somehow the for loop gets turned into a while loop. This does not cause a hang in js shell trunk (1.9).

Comment 1

[Bob Clary [:bc] (inactive)]

yeah, funky:

javascript:function f() { const x = 1; for (x in null); };f.toSource() gives

function f() {const x = 1;while (x) {}}

Comment 2

[Brian Crowder]

Doubtful this would be fixed for 1.8 unless it can be shown to be a security risk, or to break major sites.  Does it happen on trunk?

Comment 3

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

it does not occur for CVS trunk (now 1.9 branch). (See comment #0)

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

This looks a lot like bug 372565.

Comment 5

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

I verify that the fix from bug 372565 does indeed fix the hang -- I have backported the patch from that bug, will post it later after more testing.

Reopening and taking bug. Since there's a patch readily available, I don't think this should be a WONTFIX anymore, but I am open to more thoughts.

Comment 6

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

Attachment: backported patch to MOZILLA_1_8_BRANCH

This patch fixes the hang, and is backported from the corresponding trunk patch in bug 372565.

Comment 7

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

Comment on attachment 326371 [details] [diff] [review]
backported patch to MOZILLA_1_8_BRANCH

I should ask mrbkap for review instead as he created the original patch for trunk.

Also asking for approval for 1.8.1.16.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 326371 [details] [diff] [review]
backported patch to MOZILLA_1_8_BRANCH

If this does land on the 1.8 branch, please make sure you update the original bug too.

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 326371 [details] [diff] [review]
backported patch to MOZILLA_1_8_BRANCH

>Index: js/src/jsemit.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsemit.c,v
>retrieving revision 3.128.2.79
>diff -u -8 -p -r3.128.2.79 jsemit.c
>-#define JSXDR_BYTECODE_VERSION      (0xb973c0de - 16)
>+#define JSXDR_BYTECODE_VERSION      (0xb973c0de - 17)

Doesn't this need to be a unique value? I think this one has already been used on the trunk.

Comment 10

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

The value on the trunk now is 24, see http://mxr.mozilla.org/seamonkey/source/js/src/jsxdrapi.h#205

The existing value on branch is still 16, at http://mxr.mozilla.org/mozilla1.8/source/js/src/jsxdrapi.h#203

>-#define JSXDR_BYTECODE_VERSION      (0xb973c0de - 16)
>+#define JSXDR_BYTECODE_VERSION      (0xb973c0de - 17)

Should it be bumped to 25 instead?

Comment 11

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

Attachment: patch v2

Changing value of JSXDR_BYTECODE_VERSION to 29 as suggested by mrbkap. Bringing over review flag.

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 331321 [details] [diff] [review]
patch v2

Approved for 1.8.1.17, a=dveditz for release-drivers

Comment 13

[Brian Crowder]

Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.128.2.80; previous revision: 3.128.2.79
done
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.102; previous revision: 3.181.2.101
done
Checking in jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision: 3.89.2.76; previous revision: 3.89.2.75
done
Checking in jsopcode.tbl;
/cvsroot/mozilla/js/src/jsopcode.tbl,v  <--  jsopcode.tbl
new revision: 3.43.2.23; previous revision: 3.43.2.22
done
Checking in jsxdrapi.h;
/cvsroot/mozilla/js/src/jsxdrapi.h,v  <--  jsxdrapi.h
new revision: 1.14.58.13; previous revision: 1.14.58.12
done

Comment 14

[Stephen Donner [:stephend] Not actively reading bugmail]

Bob, would you mind doing the verification honors, if you have a shell with branch source handy?  Thanks!

Comment 15

[Bob Clary [:bc] (inactive)]

(In reply to comment #14)

can do this evening.

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 331321 [details] [diff] [review]
patch v2

>@@ -2722,17 +2716,17 @@ Decompile(SprintStack *ss, jsbytecode *p
>                 xval = "&&";
>                 goto do_logical_connective;
> 
>               case JSOP_FORARG:
>                 atom = GetSlotAtom(jp, js_GetArgument, GET_ARGNO(pc));
>                 LOCAL_ASSERT(atom);
>                 goto do_fornameinloop;
> 
>-              case JSOP_FORVAR:
>+              case JSOP_FORCONST:

Why was case JSOP_FORVAR: removed? That is a bug, probably bug 457093.

/be

Comment 17

[Mathieu Fenniak]

Brendan, is it possible that could also cause bug 456964?

Comment 18

[Brendan Eich [:brendan]]

If you take out the alert, does the minimal testcase in bug 456964 iloop? If not, and only if any conversion to string of the function in that testcase results in the iloop (whether via alert or just calling .toString() directly and throwing away the result), then that bug is the regression from this bug's patch.

/be

Comment 19

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-437288-01.js,v  <--  regress-437288-01.js
initial revision: 1.1

Checking in decompilation/regress-437288-02.js;
/cvsroot/mozilla/js/tests/js1_5/decompilation/regress-437288-02.js,v  <--  regress-437288-02.js
initial revision: 1.1

http://hg.mozilla.org/mozilla-central/rev/c32860dd7d9a

Comment 20

[Bob Clary [:bc] (inactive)]

(In reply to comment #18)

no iloop on mac opt shell but it does assert mac debug shell Assertion failure: top != 0, at jsopcode.c:854. testcase in a moment.

Comment 21

[Bob Clary [:bc] (inactive)]

correction: is does show for->while.

Comment 22

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

Sorry for the bad backport. :(