Last Comment Bug 652054 - Crash running jellyfish demo on 64-bit
: Crash running jellyfish demo on 64-bit
Status: VERIFIED FIXED
[sg:critical?], [qa!]
: crash, testcase, verified-beta
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- critical (vote)
: mozilla9
Assigned To: David Mandelin [:dmandelin]
:
Mentors:
http://chrysaora.com/
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-21 23:34 PDT by Boris Zbarsky [:bz] (Out June 25-July 6)
Modified: 2013-01-14 08:26 PST (History)
15 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wontfix
-
wontfix
+
wontfix
+
verified
+
verified
+
verified
unaffected


Attachments
Test case for browser (3.32 KB, text/html)
2011-05-17 12:04 PDT, Christian Holler (:decoder)
no flags Details
Test case for shell (run with -j -m) (2.28 KB, application/javascript)
2011-05-17 14:25 PDT, Christian Holler (:decoder)
no flags Details
Patch (3.08 KB, patch)
2011-05-18 18:27 PDT, David Mandelin [:dmandelin]
n.nethercote: review+
christian: approval‑mozilla‑aurora+
Details | Diff | Review
Test case for shell (run with -j -m) (69 bytes, application/javascript)
2011-08-29 16:49 PDT, Christian Holler (:decoder)
no flags Details
Test case for shell (run with -j -m) (1.26 KB, application/javascript)
2011-08-29 16:50 PDT, Christian Holler (:decoder)
no flags Details

Description Boris Zbarsky [:bz] (Out June 25-July 6) 2011-04-21 23:34:55 PDT
BUILD: Current trunk nightly and my optimized self-build

STEPS TO REPRODUCE:
1)  Disable content methodjit
2)  Load http://chrysaora.com/

EXPECTED RESULTS: no crash

ACTUAL RESULTS: crash

ADDITIONAL INFORMATION: I submitted some nightly crash reports:

https://crash-stats.mozilla.com/report/index/bp-f730c485-b35c-4ba7-92dd-7793b2110421
https://crash-stats.mozilla.com/report/index/d66398e6-085a-4218-b732-dcaab2110421
https://crash-stats.mozilla.com/report/index/674db722-65e2-4950-bd31-f30272110421

Looks like a null-deref, but can't say more than that.  We're definitely crashing on trace.  Interestingly, my 32-bit debug build does NOT hit the crash.  My 64-bit builds do.  

I don't quite know where the null-deref comes from.  Some info from gdb:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: 13 at address: 0x0000000000000000
0x0000000126ab3759 in ?? ()
(gdb) p $pc
$4 = (void (*)(void)) 0x126ab3759
(gdb) disas $pc-50 $pc+20
Dump of assembler code from 0x126ab3727 to 0x126ab376d:
0x0000000126ab3727:     add    %al,(%rax)
0x0000000126ab3729:     sbb    %al,0x66(%rax)
0x0000000126ab372c:     movd   %rsi,%mm4
0x0000000126ab3730:     mulsd  %xmm7,%xmm2
0x0000000126ab3734:     addsd  %xmm2,%xmm15
0x0000000126ab3739:     mov    $0x7ff8000000000000,%rsi
0x0000000126ab3743:     movd   %rsi,%xmm2
0x0000000126ab3748:     mulsd  %xmm5,%xmm1
0x0000000126ab374c:     addsd  %xmm1,%xmm15
0x0000000126ab3751:     xorps  %xmm1,%xmm1
0x0000000126ab3754:     cvtsd2ss %xmm15,%xmm1
0x0000000126ab3759:     movss  %xmm1,0x0(%rsi)
0x0000000126ab375e:     mov    $0x0,%esi
0x0000000126ab3763:     movd   %rsi,%xmm1
0x0000000126ab3768:     lea    0x34(%rbx),%rsi
0x0000000126ab376c:     lea    0x3c(%rbx),%rsi
(gdb) info reg rsi
rsi            0x7ff8000000000000       9221120237041090560

I guess 0x7ff8000000000000 is a non-canonical address and chances are that's what's killing us....  But not sure.

I can reproduce this at will, so if there's something else useful I can report, please let me know.
Comment 1 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-04-25 18:44:27 PDT
Relevant "native,afterdce" output:

  ------------------------------ # JSOP_ADD
      addd34 = addd addd35, muld72
  0x11ceab899  addsd xmm15, xmm2
  0x11ceab89e  movq rsi, 0x7ff8000000000000
  0x11ceab8a8  movq xmm2, rsi  <= restore immd1/*nan*/
  ------------------------------ # JSOP_GETLOCAL
  ------------------------------ # JSOP_GETLOCAL
  ------------------------------ # JSOP_MUL
      muld71 = muld cmovd203, cmovd191
  0x11ceab8ad  mulsd xmm1, xmm5
  ------------------------------ # JSOP_ADD
      addd33 = addd addd34, muld71
  0x11ceab8b1  addsd xmm15, xmm1
  ------------------------------ # JSOP_SETELEM
      std2f.tdata addq242[0] = addd33
  0x11ceab8b6  xorps xmm1, xmm1
  0x11ceab8b9  cvtsd2ss cl, xmm15
  0x11ceab8be  movss 0(rsi), xmm1
  0x11ceab8c3  movl esi, 0
  0x11ceab8c8  movq xmm1, rsi
  0x11ceab8cd  leaq rsi, 52(rbx)  <= restore addq242
  0x11ceab8d1  leaq rsi, 60(rbx)  <= restore addq240

In this case the crash is on 0x11ceab8be.

Note those restores, which come _after_ we try to write to rsi(0).  Our earlier computation of addq242 looked like this:

  ------------------------------ # JSOP_GETELEM
      inRange533 = gtui typedArrayLength56, immi10/*13*/
      xf831: xf inRange533 -> exit=0x1270ab0c0 pc=0x11986ee0a imacpc=0x0 sp+368 rp+16 BRANCH
  0x11ce6b37f  cmpl edi, 13
  0x11ce6b382  jna 0x11ce83601
  ----------------------------------- ## BEGIN exit block (LIR_xt|LIR_xf)
...
  ----------------------------------- ## END exit block 0x106ad1460
      addq242 = addq typedElems56, immq4/*52LL*/
  0x11ce6b388  movq r9, rbx
  0x11ce6b38b  addq r9, 52

(I'm not sure why we do that, since the next thing we do is to load 0(r9) into xmm5, but whatever.  In any case, it looks like the restore is restoring the right thing... but too late.
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-04-25 18:50:36 PDT
Looking back at the readlir output, the place where we compute addq242 is:

  21074: ------------------------------ # JSOP_GETELEM
  21075: inRange533 = gtui typedArrayLength56, immi10/*13*/
  21076: xf829: xf inRange533 -> exit=0x128c5c948 pc=0x11986ee0a imacpc=0x0 sp+368 rp+16 BRANCH
  21077: addq242 = addq typedElems56, immq4/*52LL*/
  21078: ldf2d193 = ldf2d.tdata addq242[0]

and then we have 600-some LIR instructions before the next use of addq242:

  21712: ------------------------------ # JSOP_SETELEM
  21713: std2f.tdata addq242[0] = addd33

which is the thing that ends up crashing.
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-04-25 19:41:48 PDT
I tried logging regalloc while running this, but 40 minutes later there's still no crash, so either it's just being too slow or it's not crashing when logging regalloc....  I think the former; it didn't crash on TMFLAGS=full either.
Comment 4 Christian Holler (:decoder) 2011-05-10 08:26:23 PDT
I was able to reproduce this, although loading takes a while with pure software rendering. I'll try to minimize this a bit now.
Comment 5 Christian Holler (:decoder) 2011-05-13 07:27:56 PDT
It takes quite a while to reproduce this because it requires WebGL and I only have software rendering available on the host machine. One automatic test takes approximately 1 minute right now inside an Xvfb. Delta is currently running on the javascripts inside the page to get rid of the unnecessary code.
Comment 6 Christian Holler (:decoder) 2011-05-17 12:04:39 PDT
Created attachment 533033 [details]
Test case for browser

Here is the reduced testcase, I couldn't get it much smaller with my automatic tools. This should crash instantly (1-2 seconds).
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-17 14:01:26 PDT
Yep, so it does.

It crashes Fx4 as well...

How can we get someone to look at this?  This crash is looking really scary to me.
Comment 8 Christian Holler (:decoder) 2011-05-17 14:25:52 PDT
Created attachment 533072 [details]
Test case for shell (run with -j -m)

I made it to turn the test into a shell testcase and reduced it further with LangDDMin. This test crashes even with methodjit enabled. S-s until this is resolved or confirmed harmless.

Backtrace:

==32287== Invalid write of size 4
==32287==    at 0x41CC833: ???
==32287==    by 0x5D55C8: js::ExecuteTrace(JSContext*, js::TraceMonitor*, nanojit::Fragment*, js::TracerState&) (jstracer.cpp:6517)
==32287==    by 0x5D5BE5: js::ExecuteTree(JSContext*, js::TraceMonitor*, js::TreeFragment*, unsigned int&, js::VMSideExit**, js::VMSideExit**) (jstracer.cpp:6623)
==32287==    by 0x5D73B5: js::RecordLoopEdge(JSContext*, js::TraceMonitor*, unsigned int&) (jstracer.cpp:7163)
==32287==    by 0x601709: js::MonitorLoopEdge(JSContext*, unsigned int&, js::InterpMode) (jstracer.cpp:17447)
==32287==    by 0x6F5D77: js::Interpret(JSContext*, js::StackFrame*, unsigned int, js::InterpMode) (jsinterp.cpp:3469)
==32287==    by 0x5FEA19: js::RecordTracePoint(JSContext*, js::TraceMonitor*, unsigned int&, bool*, bool) (jstracer.cpp:16810)
==32287==    by 0x5FF209: js::MonitorTracePoint(JSContext*, unsigned int&, bool*, void**, unsigned int*, unsigned int*, unsigned int) (jstracer.cpp:16967)
==32287==    by 0x6C2F6A: RunTracer(js::VMFrame&, js::mjit::ic::TraceICInfo&) (InvokeHelpers.cpp:967)
==32287==    by 0x6C324C: js::mjit::stubs::InvokeTracer(js::VMFrame&, js::mjit::ic::TraceICInfo*) (InvokeHelpers.cpp:1057)
==32287==    by 0x41B601B: ???
==32287==    by 0x654831: js::mjit::EnterMethodJIT(JSContext*, js::StackFrame*, void*, js::Value*) (MethodJIT.cpp:685)
==32287==  Address 0x7ff8000000000000 is not stack'd, malloc'd or (recently) free'd
==32287== 
==32287== 
==32287== Process terminating with default action of signal 11 (SIGSEGV)
==32287==  General Protection Fault
Comment 9 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-17 17:25:09 PDT
OK.  Given that, I'm picking a random assignee.  Please reassign as needed or prove this is not exploitable...
Comment 10 David Mandelin [:dmandelin] 2011-05-17 17:33:28 PDT
David's busy with IonMonkey. If you need a random assignee for JS bugs, pick me.
Comment 11 David Mandelin [:dmandelin] 2011-05-18 18:20:25 PDT
OK, here's the bug. First, bz's spew, with some comments. Read the comments from bottom to top, since that's the way nanojit generates code:

  ------------------------------ # JSOP_ADD
      addd34 = addd addd35, muld72
  0x11ceab899  addsd xmm15, xmm2
  0x11ceab89e  movq rsi, 0x7ff8000000000000
  0x11ceab8a8  movq xmm2, rsi  <= restore immd1/*nan*/
  // 6. rsi is free, so feel free to grab it as a temporary. This clobbers
  //    addq242.
  ------------------------------ # JSOP_GETLOCAL
  ------------------------------ # JSOP_GETLOCAL
  ------------------------------ # JSOP_MUL
      muld71 = muld cmovd203, cmovd191
  0x11ceab8ad  mulsd xmm1, xmm5
  ------------------------------ # JSOP_ADD
      addd33 = addd addd34, muld71
  0x11ceab8b1  addsd xmm15, xmm1
  // 5. At this point, the register state (not shown here) thinks rsi is free.
  ------------------------------ # JSOP_SETELEM
      std2f.tdata addq242[0] = addd33
  0x11ceab8b6  xorps xmm1, xmm1
  0x11ceab8b9  cvtsd2ss cl, xmm15
  0x11ceab8be  movss 0(rsi), xmm1
  // 4. Store to the array using rsi. This is supposed to represent addq242.
  //    We restore addq242. Clearly, someone has evicted addq242, but they
  //    generated the restore code for addq242 *below* its first use.
  0x11ceab8c3  movl esi, 0
  0x11ceab8c8  movq xmm1, rsi
  // 3. Above 2 instructions use rsi as a temp to load a value into xmm1.
  //    This is actually restore code for xmm1, which we are also taking
  //    here (all fp regs are in use).
  0x11ceab8cd  leaq rsi, 52(rbx)  <= restore addq242
  // 2. Now, restore rsi again. That makes no sense. Anyway, we are restoring
  //    addq242 here, so remember that.
  0x11ceab8d1  leaq rsi, 60(rbx)  <= restore addq240
  // 1. Above, restore rsi for later use, since we're about to take it here.
  //    That makes sense.

The bug is at point 2: restoring rsi *after* its use instead of just before. The cause is in Assembler::asm_store64, the LIR_std2f case. I annotate to show what happens here.

            case LIR_std2f: {
                // 1. getBaseReg picks rax. This will be used for the base
                //    address for the store. This also happens to be addq242.
                //    Fine so far.
                Register b = getBaseReg(base, d, BaseRegs);
                // 2. findRegFor gets an fp reg. 
                Register r = findRegFor(value, FpRegs);
                // 3. registerAllocTmp wants another fp reg. But there are
                //    now none left. So it evicts xmm1. In order to do this,
                //    it must generate the restore code. xmm1 holds the
                //    value 0, so it generates 
                //           0x11ceab8c3  movl esi, 0
                //           0x11ceab8c8  movq xmm1, rsi
                //    But it needs a gp temp reg for that code first. evict
                //    calls registerAllocTmp to do this. That picks rax, 
                //    evicts it, and generates spill code. But we're about
                //    to use rax, so that loses.
                Register t = registerAllocTmp(FpRegs & ~rmask(r));

                MOVSSMR(t, d, b);   // store
                CVTSD2SS(t, r);     // cvt to single-precision
                XORPS(t);           // break dependency chains
                break;
Comment 12 David Mandelin [:dmandelin] 2011-05-18 18:27:31 PDT
Created attachment 533504 [details] [diff] [review]
Patch

Simple fix. Boiled down from the previous comment, the problem is that regsterAllocTmp(FpRegs), if it needs to spill, can take a GpReg as a temporary, which may clobber the assignment made by getBaseReg(..., BaseRegs). By putting the call to getBaseReg last, we fix this, because getBaseReg(..., BaseRegs) cannot evict an FpReg (so the call to findRegFor is OK). 

This prevents the crash in this test case. I didn't add it as a new test in the patch because it seems to run a long time.

I also fixed a spew bug in CVTSD2SS.
Comment 13 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-05-18 18:43:45 PDT
Comment on attachment 533504 [details] [diff] [review]
Patch

Review of attachment 533504 [details] [diff] [review]:
-----------------------------------------------------------------

The change is unsafe, see below.

::: js/src/nanojit/NativeX64.cpp
@@ +1726,2 @@
>                  Register r = findRegFor(value, FpRegs);
>                  Register t = registerAllocTmp(FpRegs & ~rmask(r));

The comment on registerAllocTmp() says this:

"Finds a register in 'allow' to store a temporary value (one not
associated with a particular LIns), evicting one if necessary.  The
returned register is marked as being free and so can only be safely
used for code generation purposes until the regstate is next inspected
or updated."

getBaseReg() inspects and updates the regstate, so reordering is not safe
here.

I think the proper fix might be to keep the current order, but exclude |b|
from registerAllocTmp()'s allow set, eg:

  Register t = registerAllocTmp(FpRegs & ~(rmask(r)|rmask(b)));

Can you try that?  Hopefully it'll work.  If not, things get more
complicated... a call to registerAllocTmp() causing another call to
registerAllocTmp() makes me nervous.

@@ +1726,3 @@
>                  Register r = findRegFor(value, FpRegs);
>                  Register t = registerAllocTmp(FpRegs & ~rmask(r));
> +                Register b = getBaseReg(base, d, BaseRegs);

The comment on registerAllocTmp() says this:

"Finds a register in 'allow' to store a temporary value (one not
associated with a particular LIns), evicting one if necessary.  The
returned register is marked as being free and so can only be safely
used for code generation purposes until the regstate is next inspected
or updated."
Comment 14 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-05-18 18:44:55 PDT
Apologies for the repetition in that previous comment.
Comment 15 David Mandelin [:dmandelin] 2011-05-18 18:50:06 PDT
(In reply to comment #13)
> Comment on attachment 533504 [details] [diff] [review] [review]
> Patch
> 
> Review of attachment 533504 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> The change is unsafe, see below.
> 
> ::: js/src/nanojit/NativeX64.cpp
> @@ +1726,2 @@
> >                  Register r = findRegFor(value, FpRegs);
> >                  Register t = registerAllocTmp(FpRegs & ~rmask(r));
> 
> The comment on registerAllocTmp() says this:
> 
> "Finds a register in 'allow' to store a temporary value (one not
> associated with a particular LIns), evicting one if necessary.  The
> returned register is marked as being free and so can only be safely
> used for code generation purposes until the regstate is next inspected
> or updated."
> 
> getBaseReg() inspects and updates the regstate, so reordering is not safe
> here.

I believe it does happen to be safe here, because the call to getBaseReg only updates the integer regs part of the regstate, and |r| is an FpReg. But I agree that the code is pretty twitchy and a deeper fix may be wanted.

> I think the proper fix might be to keep the current order, but exclude |b|
> from registerAllocTmp()'s allow set, eg:
> 
>   Register t = registerAllocTmp(FpRegs & ~(rmask(r)|rmask(b)));
> 
> Can you try that?  Hopefully it'll work.  

That doesn't work: b is an int reg, so subtracting it from FpRegs has no effect. (And I tested just to be sure.)

> If not, things get more
> complicated... a call to registerAllocTmp() causing another call to
> registerAllocTmp() makes me nervous.

Definitely. Besides the reordering, the other immediate idea I had was to add a parameter |forbiddenReg| to registerAllocTmp, with the meaning that registerAllocTmp is not allowed to evict |forbiddenReg| via any recursive call. That would then have to be pushed down through registerAlloc and evict.
Comment 16 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-05-18 18:54:36 PDT
Comment on attachment 533504 [details] [diff] [review]
Patch

> I believe it does happen to be safe here, because the call to getBaseReg only
> updates the integer regs part of the regstate, and |r| is an FpReg.

Hmm, true, I give r+ so long as you write a comment explaining and include a pointer to this bug :)
Comment 17 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-05-18 18:55:01 PDT
Hmm, since it's a security bug, don't worry about the pointer to this bug.
Comment 18 David Anderson [:dvander] 2011-05-18 19:37:33 PDT
Or maybe mask out the temp reg from the other functions, just to be safe/explicit.
Comment 19 David Mandelin [:dmandelin] 2011-05-19 10:58:50 PDT
http://hg.mozilla.org/tracemonkey/rev/354f0ee56b8e
Comment 20 Chris Leary [:cdleary] (not checking bugmail) 2011-05-23 14:10:18 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/354f0ee56b8e
Comment 21 Christian Holler (:decoder) 2011-07-30 11:25:52 PDT
My testcase from comment #8 still crashes mozilla-central, mozilla-inbound and tracemonkey shells. Reopening this bug, as it doesn't seem to be fixed.
Comment 22 Daniel Veditz [:dveditz] 2011-08-10 16:39:09 PDT
Are you getting the same exploitable-looking symptoms from comment 8 or is it possibly a different crash?
Comment 23 Christian Holler (:decoder) 2011-08-10 16:49:51 PDT
Yes, the backtrace is exactly the same as in comment 8, involving 0x7ff8000000000000.
Comment 24 Christian Holler (:decoder) 2011-08-10 16:56:15 PDT
I analyzed this further, the patch was probably accidentally reverted here:

changeset:   70616:a163ff4b8bf1
user:        Rick Reitmaier <rreitmai@adobe.com>
date:        Fri May 20 16:01:11 2011 -0700
summary:     Bug 609393 - resourceConsistencyCheck x87 stack assert can fire (r+wmaddox)

Also, the test should be included in the JS testsuite to avoid such situations.
Comment 25 Johnny Stenback (:jst, jst@mozilla.com) 2011-08-11 13:22:16 PDT
This seems sg:moderate given that this is in a non-default configuration.
Comment 26 Christian Holler (:decoder) 2011-08-11 13:44:00 PDT
The shell testcase I posted in comment 8 works with both JITs enabled, unlike the original website problem reported here. So this affects the default configuration, as far as I can see.
Comment 27 Johnny Stenback (:jst, jst@mozilla.com) 2011-08-11 14:12:30 PDT
Ok, sg:critical? it is, again, then. Should the title be updated here then too?
Comment 28 Christian Holler (:decoder) 2011-08-11 14:22:44 PDT
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #27)
> Ok, sg:critical? it is, again, then. Should the title be updated here then
> too?

True :) Removed the misleading title.
Comment 29 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-08-11 15:03:27 PDT
The title was based on the simplest steps to reproduce.  But yes, this can trigger any time we're tracing....
Comment 30 David Mandelin [:dmandelin] 2011-08-29 16:13:41 PDT
(In reply to Christian Holler (:decoder) from comment #24)
> Also, the test should be included in the JS testsuite to avoid such
> situations.

The test seems to be very long-running or even infinite? Do you have a fast version? I thought about it while writing the patch but the bug is very subtle and writing a short JS program to expose it seems hard.
Comment 31 Christian Holler (:decoder) 2011-08-29 16:49:07 PDT
Created attachment 556712 [details]
Test case for shell (run with -j -m)

Here's a much shorter testcase. Additionally, this test case does not loop infinitely with the fix applied. On both crashing and fixed version, it should terminate immediately.
Comment 32 Christian Holler (:decoder) 2011-08-29 16:50:28 PDT
Created attachment 556714 [details]
Test case for shell (run with -j -m)

Had the wrong file attached, this should be the right one...
Comment 33 David Mandelin [:dmandelin] 2011-08-29 18:46:37 PDT
Relanded the right way:

http://hg.mozilla.org/projects/nanojit-central/rev/e9ffcbaa90bc
Comment 34 David Mandelin [:dmandelin] 2011-08-29 18:51:55 PDT
Relanded to m-i with test case, so it's fixed right away and for the test case.
Comment 35 David Mandelin [:dmandelin] 2011-08-29 18:54:41 PDT
The patch wasn't backed out before Beta, but it was for Aurora.
Comment 36 christian 2011-08-30 14:58:15 PDT
It isn't clear from this bug what the proper action is. From the comments and status flags it sounds like you might want to be asking for approval on beta instead?
Comment 37 David Mandelin [:dmandelin] 2011-08-30 16:35:36 PDT
(In reply to Christian Legnitto [:LegNeato] from comment #36)
> It isn't clear from this bug what the proper action is. From the comments
> and status flags it sounds like you might want to be asking for approval on
> beta instead?

The bug was fixed, and then the fix was unintentionally reverted. The fix made it to Beta, but not the revert. Both the fix and the revert made it to Aurora. So only Aurora needs a relanding.
Comment 38 christian 2011-08-30 16:52:59 PDT
Ok, then the status flags were flipped, causing the confusion. Approved for aurora and I flipped the flags
Comment 39 David Mandelin [:dmandelin] 2011-09-06 13:40:24 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/96c12e1c1c80
Comment 40 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-08 10:29:37 PDT
http://hg.mozilla.org/mozilla-central/rev/663af1e15f0f
Comment 41 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-26 09:24:00 PDT
(In reply to Boris Zbarsky (:bz) from comment #0)
> BUILD: Current trunk nightly and my optimized self-build
> 
> STEPS TO REPRODUCE:
> 1)  Disable content methodjit
> 2)  Load http://chrysaora.com/

Given this testcase, is it possible to verify this in Firefox 7.0RC?
Comment 42 Martin Stránský 2011-09-27 03:37:07 PDT
The testcase still crashes for me, Firefox 7.0RC, Fedora15/x86_64, built with --disable-methodjit.
Comment 43 Christian Holler (:decoder) 2011-09-27 12:22:04 PDT
(In reply to David Mandelin from comment #35)
> The patch wasn't backed out before Beta, but it was for Aurora.

I'm not sure if that is correct. I just retested Aurora, Beta (Aurora in that comment and Release (Beta in that comment) JS shells.

Aurora and Beta are ok, Release (Fx7) still crashes on this.
Comment 44 Daniel Veditz [:dveditz] 2011-09-27 13:37:38 PDT
Unless you're saying Nightly is busted we should leave the bug "fixed" and use the branch "status" fields to note this fix didn't land on the "7" branch.

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #41)
> (In reply to Boris Zbarsky (:bz) from comment #0)
> > STEPS TO REPRODUCE:
> > 1)  Disable content methodjit
> > 2)  Load http://chrysaora.com/
> 
> Given this testcase, is it possible to verify this in Firefox 7.0RC?

Never pay too much attention to any comment 0. In this case it's bz so more reliable than your average bug author, but the bug did spring two additional minimized testcases (comment 6, comment 8).
Comment 45 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-27 13:51:22 PDT
Thanks Daniel.

qa+ for verification in Firefox 8 with the testcase in comment 6.
Comment 46 Christian Holler (:decoder) 2011-09-27 15:18:17 PDT
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #45)
> qa+ for verification in Firefox 8 with the testcase in comment 6.

You should also consider using the test from comment 31. It is the same as in comment 8 (JS shell test, not requiring a browser at all), but it terminates unlike the one in comment 8. This test is already in our test suite (jit-test/tests/basic/bug652054.js).
Comment 47 Vlad [QA] 2012-01-25 04:57:15 PST
Hi guys.
I wanted to test this using the testcase from comment 6 and Firefox freezes every time.
This happened with Firefox 7.0.1, 8.0.1, 9.0.1 and also on Firefox 10b6.
I've done the test on MacOS X 10.6.8.
There is another test case that I should use?

Thanks
Comment 48 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-01-25 07:16:59 PST
(In reply to Vlad [QA] from comment #47)
> Hi guys.
> I wanted to test this using the testcase from comment 6 and Firefox freezes
> every time.
> This happened with Firefox 7.0.1, 8.0.1, 9.0.1 and also on Firefox 10b6.
> I've done the test on MacOS X 10.6.8.
> There is another test case that I should use?
> 
> Thanks

If Firefox is freezing every time you try to load this test it's probably worth a follow-up bug. As long as it doesn't trigger Breakpad this is likely resolved.

@Johnny, can you comment on this ^
Comment 49 Johnny Stenback (:jst, jst@mozilla.com) 2012-01-25 07:34:47 PST
Yeah, that sounds like a different bug.
Comment 50 Vlad [QA] 2012-01-26 02:11:34 PST
I have retested this on:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0.1) Gecko/20100101 Firefox/8.0.1
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0.1) Gecko/20100101 Firefox/9.0.1
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0) Gecko/20100101 Firefox/10.0 beta 6

On each build I get an "Unresponsive script" error and if I choose to continue the script Firefox freezes but no Brackpad dialog appears. 
Considering this and the comments from above, setting resolution to Verified Fixed.

Should I file a new bug regarding the freezes ?
Comment 51 Christian Holler (:decoder) 2013-01-14 08:26:29 PST
A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug652054.js.

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