Last Comment Bug 728342 - Crash near null with testcase involving mjitChunkLimit
: Crash near null with testcase involving mjitChunkLimit
Status: VERIFIED FIXED
[sg:dos] js-triage-needed
: crash, regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
: -- critical (vote)
: mozilla13
Assigned To: Brian Hackett (:bhackett)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: jsfunfuzz 706914
  Show dependency treegraph
 
Reported: 2012-02-17 11:44 PST by Gary Kwong [:gkw] [:nth10sd]
Modified: 2012-04-19 11:53 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
fixed
fixed
unaffected


Attachments
patch #1 (905 bytes, patch)
2012-02-20 08:21 PST, Brian Hackett (:bhackett)
dvander: review+
jst: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
patch #2 (3b8ad7252ccb) (10.16 KB, patch)
2012-02-21 06:20 PST, Brian Hackett (:bhackett)
dvander: review+
jst: approval‑mozilla‑aurora-
Details | Diff | Splinter Review

Description Gary Kwong [:gkw] [:nth10sd] 2012-02-17 11:44:10 PST
mjitChunkLimit(6)
try {
    (function() {
        function a([], x) {
            x.x::c
        }
        a()
    })()
} catch (e) {}

crashes js debug and opt 32-bit shell on m-c changeset 78fde7e54d92 with -m, -a and -n near null.

Locking s-s just-in-case even though this seems like a null deref.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   84929:d0c192e5bd41
user:        Brian Hackett
date:        Wed Jan 18 16:40:18 2012 -0800
summary:     Compile large scripts in chunks, bug 706914. r=dvander

Opt shell stack trace:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x0000000c
0x00f597e1 in ?? ()
(gdb) bt
#0  0x00f597e1 in ?? ()
#1  0x01b0af00 in ?? ()
Previous frame inner to this frame (gdb could not unwind past this frame)
(gdb) x/i $pc
0xf597e1:	mov    0xc(%esi),%ebx
(gdb) x/b $esi
0x0:	Cannot access memory at address 0x0
(gdb) x/b $ebx
0xb390d8:	0x02
Comment 1 Brian Hackett (:bhackett) 2012-02-20 08:21:44 PST
Created attachment 598886 [details] [diff] [review]
patch #1

The closure analysis done for NAME ops could return the current script, which is a problem if the script has not been marked as an outer function as its active call object will not be updated in prologues/epilogues.

This was not caused by chunk compilation, but was induced by it since this situation could not come up with whole method compilation (which would abort on the QNAMECONST and other opcodes which make the frontend throw up its hands in resolving local name accesses).  I'll put together another patch to make sure we do not do any compilation in scripts which contain any uncompileable opcodes (generalizing bug 726799), which should cut off any more strange new cases which could not have occurred before chunked compilation.
Comment 2 Brian Hackett (:bhackett) 2012-02-21 06:20:17 PST
Created attachment 599141 [details] [diff] [review]
patch #2 (3b8ad7252ccb)

Patch to ensure no parts of scripts are compiled when they contain uncompileable opcodes.  This is pretty ugly but I think it's a good fail safe to put in and avoid hard-to-fuzz problems with weird rare opcodes.
Comment 3 Brian Hackett (:bhackett) 2012-02-22 11:41:07 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/0457004daa8c
Comment 4 Brian Hackett (:bhackett) 2012-02-22 11:43:26 PST
Comment on attachment 599141 [details] [diff] [review]
patch #2 (3b8ad7252ccb)

[Approval Request Comment]
User impact if declined: Defensive patch against potential crashes caused by compiling part of a script which would not have been compiled at all before bug 706914.
Risk to taking this patch (and alternatives if risky): Should be low risk, patch changes a fair bit of code but should not change semantics at all. If this is too much for aurora, patch #1 is more restricted (just fixes a crash).
Comment 5 Brian Hackett (:bhackett) 2012-02-22 12:58:29 PST
Followup fix to compile scripts containing decomposed opcodes.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf3651d2e149
Comment 6 Ed Morley [:emorley] 2012-02-22 14:16:31 PST
(In reply to Brian Hackett (:bhackett) from comment #5)
> Followup fix to compile scripts containing decomposed opcodes.

Will this fix these regressions?
https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/CVxkOgplIuE
Comment 7 Brian Hackett (:bhackett) 2012-02-22 16:43:53 PST
(In reply to Ed Morley [:edmorley] from comment #6)
> (In reply to Brian Hackett (:bhackett) from comment #5)
> > Followup fix to compile scripts containing decomposed opcodes.
> 
> Will this fix these regressions?
> https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/
> CVxkOgplIuE

Yeah, it should.  The first push was causing us to not compile large portions of many benchmarks.
Comment 8 Ed Morley [:emorley] 2012-02-22 16:52:01 PST
(In reply to Brian Hackett (:bhackett) from comment #7)
> Yeah, it should.  The first push was causing us to not compile large
> portions of many benchmarks.

Great :-)

Graphs-new has since confirmed this, eg:
http://graphs-new.mozilla.org/graph.html#tests=[[74,131,15]]&sel=1329852891000,1330025691000&displayrange=7&datatype=running
and
http://graphs-new.mozilla.org/graph.html#tests=[[77,131,15]]&sel=1329852891000,1330025691000&displayrange=7&datatype=running
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2012-02-23 14:49:42 PST
Comment on attachment 599141 [details] [diff] [review]
patch #2 (3b8ad7252ccb)

Drivers decided to take patch 1 for aurora here. Minusing this one, and I'll approve the first patch.
Comment 11 Johnny Stenback (:jst, jst@mozilla.com) 2012-02-23 14:49:57 PST
Comment on attachment 598886 [details] [diff] [review]
patch #1

[Triage Comment]
Comment 12 Brian Hackett (:bhackett) 2012-02-24 05:58:47 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/74efe408346a
Comment 13 Al Billings [:abillings] 2012-03-16 15:12:20 PDT
verified fixed on trunk in nightly js shell.

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