Last Comment Bug 437288 - [1.8 branch] Hang involving a for loop turning into a while loop
: [1.8 branch] Hang involving a for loop turning into a while loop
Status: VERIFIED FIXED
: hang, testcase, verified1.8.1.17
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 1.8 Branch
: x86 Mac OS X
: -- critical (vote)
: ---
Assigned To: Gary Kwong [:gkw] [:nth10sd]
:
Mentors:
Depends on: 372565 457093 457417
Blocks: jsfunfuzz
  Show dependency treegraph
 
Reported: 2008-06-04 13:47 PDT by Gary Kwong [:gkw] [:nth10sd]
Modified: 2008-09-27 05:47 PDT (History)
11 users (show)
bob: in‑testsuite+
bob: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
backported patch to MOZILLA_1_8_BRANCH (9.35 KB, patch)
2008-06-23 13:45 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details | Diff | Review
patch v2 (9.35 KB, patch)
2008-07-25 10:50 PDT, Gary Kwong [:gkw] [:nth10sd]
gary: review+
dveditz: approval1.8.1.17+
Details | Diff | Review

Description Gary Kwong [:gkw] [:nth10sd] 2008-06-04 13:47:30 PDT
(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:] 2008-06-04 13:58:19 PDT
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 2008-06-04 14:01:25 PDT
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] 2008-06-04 14:06:01 PDT
it does not occur for CVS trunk (now 1.9 branch). (See comment #0)
Comment 4 Blake Kaplan (:mrbkap) (please use needinfo!) 2008-06-23 08:04:44 PDT
This looks a lot like bug 372565.
Comment 5 Gary Kwong [:gkw] [:nth10sd] 2008-06-23 11:43:15 PDT
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] 2008-06-23 13:45:38 PDT
Created attachment 326371 [details] [diff] [review]
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] 2008-06-23 14:01:20 PDT
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) (please use needinfo!) 2008-06-23 14:04:23 PDT
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] 2008-07-16 15:09:23 PDT
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] 2008-07-16 21:06:37 PDT
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] 2008-07-25 10:50:26 PDT
Created attachment 331321 [details] [diff] [review]
patch v2

Changing value of JSXDR_BYTECODE_VERSION to 29 as suggested by mrbkap. Bringing over review flag.
Comment 12 Daniel Veditz [:dveditz] 2008-08-04 11:26:27 PDT
Comment on attachment 331321 [details] [diff] [review]
patch v2

Approved for 1.8.1.17, a=dveditz for release-drivers
Comment 13 Brian Crowder 2008-08-07 09:24:14 PDT
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] 2008-08-29 17:22:19 PDT
Bob, would you mind doing the verification honors, if you have a shell with branch source handy?  Thanks!
Comment 15 Bob Clary [:bc:] 2008-08-30 12:55:03 PDT
(In reply to comment #14)

can do this evening.
Comment 16 Brendan Eich [:brendan] 2008-09-25 18:04:24 PDT
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 2008-09-25 18:23:22 PDT
Brendan, is it possible that could also cause bug 456964?
Comment 18 Brendan Eich [:brendan] 2008-09-25 18:28:35 PDT
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:] 2008-09-26 00:31:35 PDT
/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:] 2008-09-26 00:36:33 PDT
(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:] 2008-09-26 00:53:17 PDT
correction: is does show for->while.
Comment 22 Gary Kwong [:gkw] [:nth10sd] 2008-09-26 12:55:27 PDT
Sorry for the bad backport. :(

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