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

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: gkw, Assigned: gkw)

Tracking

(Blocks: 1 bug, {hang, testcase, verified1.8.1.17})

1.8 Branch
x86
Mac OS X
hang, testcase, verified1.8.1.17
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 years ago
(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

9 years ago
yeah, funky:

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

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

Comment 2

9 years ago
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?
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → WONTFIX
(Assignee)

Comment 3

9 years ago
it does not occur for CVS trunk (now 1.9 branch). (See comment #0)
This looks a lot like bug 372565.
(Assignee)

Comment 5

9 years ago
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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(Assignee)

Comment 6

9 years ago
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.
Assignee: general → nth10sd
Status: REOPENED → ASSIGNED
Attachment #326371 - Flags: review?(brendan)
(Assignee)

Comment 7

9 years ago
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.
Attachment #326371 - Flags: review?(mrbkap)
Attachment #326371 - Flags: review?(brendan)
Attachment #326371 - Flags: approval1.8.1.16?
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.
Attachment #326371 - Flags: review?(mrbkap) → review+
Depends on: 372565
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.
(Assignee)

Comment 10

9 years ago
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?
(Assignee)

Comment 11

9 years ago
Created attachment 331321 [details] [diff] [review]
patch v2

Changing value of JSXDR_BYTECODE_VERSION to 29 as suggested by mrbkap. Bringing over review flag.
Attachment #326371 - Attachment is obsolete: true
Attachment #331321 - Flags: review+
Attachment #331321 - Flags: approval1.8.1.17?
Attachment #326371 - Flags: approval1.8.1.17?
(Assignee)

Updated

9 years ago
Attachment #326371 - Flags: review+
Comment on attachment 331321 [details] [diff] [review]
patch v2

Approved for 1.8.1.17, a=dveditz for release-drivers
Attachment #331321 - Flags: approval1.8.1.17? → approval1.8.1.17+

Comment 13

9 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago9 years ago
Keywords: fixed1.8.1.17
Resolution: --- → FIXED
Bob, would you mind doing the verification honors, if you have a shell with branch source handy?  Thanks!

Comment 15

9 years ago
(In reply to comment #14)

can do this evening.

Updated

9 years ago
Flags: in-testsuite?
Keywords: fixed1.8.1.17 → verified1.8.1.17
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
Depends on: 457093
No longer depends on: 457093

Comment 17

9 years ago
Brendan, is it possible that could also cause bug 456964?
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

9 years ago
/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
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus-

Comment 20

9 years ago
(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

9 years ago
correction: is does show for->while.

Updated

9 years ago
Depends on: 457093
(Assignee)

Comment 22

9 years ago
Sorry for the bad backport. :(

Updated

9 years ago
Depends on: 457417
You need to log in before you can comment on or make changes to this bug.