737388 - Crash [@ JSString::isAtom] or Opt-Crash [@ JSCompartment::wrap]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following test crashes on mozilla-central revision 58a2cd0203ee (options -m -n):


function toPrinted(value) {
  if (typeof value == "xml") 
    value = value.toXMLString();
}
function reportCompare (expected, actual, description) {
  if (typeof description == "undefined")
    description = '';
    "', Actual value '" + toPrinted(actual) + "' ";
}
var UBound = 0;
var actualvalues = [];
var expect= '';
var expectedvalues = [];
addThis();
test();
function addThis() {
  expectedvalues[UBound] = expect;
  UBound++;
}
function test() {
  for (var i=(0); i<UBound; i++)
    reportCompare(expectedvalues[i], actualvalues[i], (arguments)[i]);
  void Number.NEGATIVE_INFINITY || -1, test();
}


Crash Trace:

==13246== Invalid read of size 8
==13246==    at 0x422C48: JSString::isAtom() const (String.h:381)
==13246==    by 0x4255D5: js::CompartmentChecker::check(JSString*) (jscntxtinlines.h:184)
==13246==    by 0x42566B: js::CompartmentChecker::check(JS::Value const&) (jscntxtinlines.h:192)
==13246==    by 0x445F44: void js::assertSameCompartment<JS::Value>(JSContext*, JS::Value) (jscntxtinlines.h:254)
==13246==    by 0x509D79: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2992)
==13246==    by 0x4FB710: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:469)
==13246==    by 0x4FC254: js::ExecuteKernel(JSContext*, JSScript*, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) (jsinterp.cpp:667)
==13246==    by 0x4FC462: js::Execute(JSContext*, JSScript*, JSObject&, JS::Value*) (jsinterp.cpp:709)
==13246==    by 0x441C92: JS_ExecuteScript (jsapi.cpp:5232)
==13246==    by 0x408A0B: Process(JSContext*, JSObject*, char const*, bool) (js.cpp:478)
==13246==    by 0x412FF8: ProcessArgs(JSContext*, JSObject*, js::cli::OptionParser*) (js.cpp:4736)
==13246==    by 0x413270: Shell(JSContext*, js::cli::OptionParser*, char**) (js.cpp:4819)
==13246==  Address 0x2 is not stack'd, malloc'd or (recently) free'd


Opt crash trace:

==11287== Invalid read of size 8
==11287==    at 0x437C17: JSCompartment::wrap(JSContext*, JS::Value*) (jscompartment.cpp:183)
==11287==    by 0x44B7DE: AppendWrappedArg::operator()(unsigned int, JS::Value*) (jsexn.cpp:324)
==11287==    by 0x44BF43: InitExnPrivate(JSContext*, JSObject*, JSString*, JSString*, unsigned int, JSErrorReport*, int) (Stack-inl.h:227)
==11287==    by 0x44C369: js_ErrorToException(JSContext*, char const*, JSErrorReport*, JSErrorFormatString const* (*)(void*, char const*, unsigned int), void*) (jsexn.cpp:1179)
==11287==    by 0x433CB3: ReportError(JSContext*, char const*, JSErrorReport*, JSErrorFormatString const* (*)(void*, char const*, unsigned int), void*) (jscntxt.cpp:339)
==11287==    by 0x434EC7: js_ReportErrorNumberVA(JSContext*, unsigned int, JSErrorFormatString const* (*)(void*, char const*, unsigned int), void*, unsigned int, int, __va_list_tag*) (jscntxt.cpp:723)
==11287==    by 0x41A184: JS_ReportErrorNumber (jsapi.cpp:6111)
==11287==    by 0x5EF8CC: js::mjit::stubs::HitStackQuota(js::VMFrame&) (InvokeHelpers.cpp:203)
==11287==    by 0x403CCAA: ???
==11287==    by 0x573E5B: js::mjit::EnterMethodJIT(JSContext*, js::StackFrame*, void*, JS::Value*, bool) (MethodJIT.cpp:1052)
==11287==    by 0x573FA2: js::mjit::JaegerShot(JSContext*, bool) (MethodJIT.cpp:1111)
==11287==    by 0x47EE8D: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2730)
==11287==  Address 0x0 is not stack'd, malloc'd or (recently) free'd

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I added a print(description) statement to the top of reportCompare and then was able to bisect it to this:

changeset:   89455:d2107141265f
user:        Luke Wagner <[e-mail]>
date:        Tue Jan 17 16:35:12 2012 -0800
summary:     Bug 730497 - create arguments object eagerly (r=bhackett)

I tried the patch in bug 737575, but it didn't fix the problem.

It seems like when we access arguments[i] in the JIT, we're getting the value 2 tagged as a string. Marking s-s just in case.

Comment 2

[Luke Wagner [:luke]]

Attachment: fix and test

Arg, I removed this
  https://hg.mozilla.org/mozilla-central/rev/85bef04d1258#l14.31
in the eager args earlier when I had a slightly different approach and then forgot to put it back.

Brian: I don't see why being heavyweight or having an argsobj would force fp->u.numActual to be valid, so, the patch initializes fp->u.numActual in both cases.  Do you know of any good reasons?

Comment 3

[Brian Hackett [Laid off!]]

Comment on attachment 607878 [details] [diff] [review]
fix and test

That code was going back to when the fp's union could store either nactual or the args obj pointer itself.  If the function is heavyweight then it has an args obj and that pointer should not have been scribbled over by the prologue.  Optimization of arguments accesses for the script would have been avoided in such cases anyways, as it was disabled for any script which args objs have been created for.

Comment 4

[Luke Wagner [:luke]]

Ah, you're right about the argsobj + the old frame union.  Heavyweightness never forced an arguments object so I think your second explanation (that the optimization was off for heavyweight functions) would be the reason.

Comment 5

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ed51dce1e43

Comment 6

[Daniel Veditz [:dveditz]]

Looks like we should get this fixed on beta and aurora, but don't need it in Firefox 11 or earlier.

Comment 7

[Luke Wagner [:luke]]

The bug is only on m-c.

Comment 10

[Luke Wagner [:luke]]

https://hg.mozilla.org/mozilla-central/rev/9ed51dce1e43