Last Comment Bug 736012 - Assertion failure: enumerators == cx->enumerators, at jsinterp.cpp:453 or Crash [@ js_SuppressDeletedProperty]
: Assertion failure: enumerators == cx->enumerators, at jsinterp.cpp:453 or Cra...
Status: VERIFIED FIXED
[sg:critical][advisory-tracking+]
: assertion, crash, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
: -- critical (vote)
: mozilla14
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on:
Blocks: langfuzz
  Show dependency treegraph
 
Reported: 2012-03-15 02:10 PDT by Christian Holler (:decoder)
Modified: 2013-01-14 08:15 PST (History)
8 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
unaffected


Attachments
fix and test (1.44 KB, patch)
2012-03-15 11:41 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
fix (4.76 KB, patch)
2012-03-15 13:01 PDT, Luke Wagner [:luke]
bhackett1024: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description Christian Holler (:decoder) 2012-03-15 02:10:31 PDT
The following test asserts/crashes on mozilla-central revision dfcb11712ec2 (options -m -n):


var lfcode = new Array();
lfcode.push("\
function fatty() {\
    try { fatty(); } catch (e) {\
        for each (foo in [1]) {}\
    }\
}\
fatty();\
");
lfcode.push("gc()");
lfcode.push("");
while (true) {
        var file = lfcode.shift(); if (file == undefined) { break; }
        evaluate(file);
}


Crash trace:

==63414== Invalid read of size 1
==63414==    at 0x4848F0: js_SuppressDeletedProperty(JSContext*, JSObject*, long) (jsiter.cpp:1002)
==63414==    by 0x424B31: js::array_shift(JSContext*, unsigned int, JS::Value*) (jsarray.cpp:2559)
==63414==    by 0x483018: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:314)
==63414==    by 0x47EB8D: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2710)
==63414==    by 0x483AFD: js::Execute(JSContext*, JSScript*, JSObject&, JS::Value*) (jsinterp.cpp:668)
==63414==    by 0x4187F6: JS_ExecuteScript (jsapi.cpp:5275)
==63414==    by 0x408294: Process(JSContext*, JSObject*, char const*, bool) (js.cpp:478)
==63414==    by 0x40AF03: Shell(JSContext*, js::cli::OptionParser*, char**) (js.cpp:4751)
==63414==    by 0x40B5C5: main (js.cpp:5044)
==63414==  Address 0x30 is not stack'd, malloc'd or (recently) free'd


Possibly related to bug 734987?

I'm marking this S-s because the bug requires "gc()" although the crash looks like a null-pointer crash. Please triage and remove S-s if appropriate :)
Comment 1 Luke Wagner [:luke] 2012-03-15 11:40:22 PDT
This is another pre-existing issue flushed out by bug 732744.  In this case, an error while jaeger-shotting at a loop header pops the frame without jumping to 'error' first.

IIUC, this is on aurora.  I'd like to let the fix bake a bit on m-c, though.
Comment 2 Luke Wagner [:luke] 2012-03-15 11:41:26 PDT
Created attachment 606310 [details] [diff] [review]
fix and test
Comment 3 Luke Wagner [:luke] 2012-03-15 12:16:47 PDT
Comment on attachment 606310 [details] [diff] [review]
fix and test

Erm, nevermind, that's totally wrong.
Comment 4 Luke Wagner [:luke] 2012-03-15 13:01:09 PDT
Created attachment 606332 [details] [diff] [review]
fix

So, actually reading the comment this time, Jaeger_Throwing indicates that the current frame has finished.  Thus it is wrong to 'goto error' in general for Jaeger_Throwing.  The general case of exception-thrown-in-jit-code is handled corectly by FindExceptionHandler.  It is only the special case of "failure to reserve stack space" in CheckStackAndEnterMethodJit that leaves us in this weird state.  Unfortunately, I don't see a better fix than introducing yet another enum return.  Let me know if you have any bright ideas Brian.
Comment 6 Luke Wagner [:luke] 2012-03-16 12:20:37 PDT
Comment on attachment 606332 [details] [diff] [review]
fix

[Approval Request Comment]
Regression caused by (bug #): bug 732744
User impact if declined: crash
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): small patch, has some risk, but we still have a lot of time for this to bake.
Comment 7 Alex Keybl [:akeybl] 2012-03-16 13:07:34 PDT
(In reply to Luke Wagner [:luke] from comment #6)
> Risk to taking this patch (and alternatives if risky): small patch, has some
> risk, but we still have a lot of time for this to bake.

1) Do we expect this crash to be more pronounced with our GA? Right now there's only a handful of crashes on 13
2) Do you expect that any possible regressions will be apparent over the 11 weeks before release?

Thanks!
Comment 8 Luke Wagner [:luke] 2012-03-16 15:25:07 PDT
(In reply to Alex Keybl [:akeybl] from comment #7)
1) no
2) yes.  11 weeks of fuzzing should be plenty :)
Comment 9 Alex Keybl [:akeybl] 2012-03-16 15:58:35 PDT
Comment on attachment 606332 [details] [diff] [review]
fix

[Triage Comment]
Approved for Aurora 13 since we believe we'll find any regressions prior to release.
Comment 10 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-03-17 17:08:41 PDT
https://hg.mozilla.org/mozilla-central/rev/a2128894e47f
Comment 12 [On PTO until 6/29] 2012-05-18 17:25:07 PDT
Was Firefox 12 unaffected by this?
Comment 13 Christian Holler (:decoder) 2013-01-14 08:15:31 PST
A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/testBug736012.js.

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