792220 - IonMonkey: Assertion failure: !cx->compartment->activeAnalysis, at jsinterp.cpp:337 or Crash [@ js::types::CompilerOutput::isValid]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision e4757379b99a (run with --ion-eager):


var p = Proxy.create({
    has : function(id) {}
});
Object.prototype.__proto__ = p;
function f() {
        if (/a/.exec("a"))
            (null).default++;
}
delete RegExp.prototype.test;
f();

Comment 1

[Christian Holler (:decoder)]

Valgrind shows a lot of fun stuff here, e.g.:

==16860== Invalid read of size 1
==16860==    at 0x80C405F: js::types::TypeCompartment::addPendingRecompile(JSContext*, js::types::RecompileInfo const&) (jsinfer.cpp:2455)
==16860==    by 0x80C4607: AddPendingRecompile(JSContext*, JSScript*, unsigned char*, RecompileKind) (jsinfer.cpp:2004)
==16860==    by 0x806CB59: js::types::TypeCompartment::resolvePending(JSContext*) (jsinferinlines.h:1010)
==16860==    by 0x807664D: js::types::TypeSet::addType(JSContext*, js::types::Type) (jsinferinlines.h:1329)
==16860==    by 0x80BE45F: js::types::TypeMonitorResult(JSContext*, JSScript*, unsigned char*, JS::Value const&) (jsinfer.cpp:5325)
==16860==    by 0x80D479F: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinferinlines.h:792)
==16860==    by 0x8C5B0F3: ???
==16860==  Address 0x8c8f984 is 4 bytes before a block of size 32 alloc'd
==16860==    at 0x7C01876: malloc (vg_replace_malloc.c:236)
==16860==    by 0x8229E24: js::VectorImpl<js::types::CompilerOutput, 0u, js::TempAllocPolicy, false>::growTo(js::Vector<js::types::CompilerOutput, 0u, js::TempAllocPolicy>&, unsigned int) (Utility.h:153)
==16860==    by 0x82A82F7: _ZN2js6VectorINS_5types14CompilerOutputELj0ENS_15TempAllocPolicyEE13growStorageByEj.clone.330 (Vector.h:612)
==16860==    by 0x82A8A7D: bool js::ion::IonCompile<&(js::ion::TestCompiler(js::ion::IonBuilder*, js::ion::MIRGraph*, js::ion::AutoDestroyAllocator&))>(JSContext*, JSScript*, JSFunction*, unsigned char*, bool) (Vector
.h:772)
==16860==    by 0x82A8C96: js::ion::MethodStatus js::ion::Compile<&(js::ion::TestCompiler(js::ion::IonBuilder*, js::ion::MIRGraph*, js::ion::AutoDestroyAllocator&))>(JSContext*, JSScript*, JSFunction*, unsigned char*,
 bool) (Ion.cpp:1154)
==16860==    by 0x82A8DD6: js::ion::CanEnter(JSContext*, JSScript*, js::StackFrame*, bool) (Ion.cpp:1252)
==16860==    by 0x80E01BF: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:297)
==16860==    by 0x80E03B6: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jsinterp.cpp:378)
==16860==    by 0x80E09F1: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) (jsinterp.h:109)
==16860==    by 0x811A588: Trap1(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<int>, JS::Value*) (jsproxy.cpp:654)
==16860==    by 0x92126BF: ???


There was also some glibc abort within the output during minimization, so I'm assuming sec-critical here.

Comment 2

[Christian Holler (:decoder)]

I also got a test with this crash trace instead, but it also involves a Proxy and triggers the same assertion in debug builds:


==18606== Invalid read of size 1
==18606==    at 0x64C73F: bool js::ion::IonCompile<&(js::ion::TestCompiler(js::ion::IonBuilder*, js::ion::MIRGraph*, js::ion::AutoDestroyAllocator&))>(JSContext*, JSScript*, JSFunction*, unsigned char*, bool) (jsinferinlines.h:389)
==18606==    by 0x64C9FC: js::ion::MethodStatus js::ion::Compile<&(js::ion::TestCompiler(js::ion::IonBuilder*, js::ion::MIRGraph*, js::ion::AutoDestroyAllocator&))>(JSContext*, JSScript*, JSFunction*, unsigned char*, bool) (Ion.cpp:1154)
==18606==    by 0x64CB74: js::ion::CanEnter(JSContext*, JSScript*, js::StackFrame*, bool) (Ion.cpp:1252)
==18606==    by 0x492672: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2479)
==18606==    by 0x49659C: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:324)
==18606==    by 0x497531: js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) (jsinterp.cpp:509)
==18606==    by 0x41910A: JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, unsigned short const*, unsigned long, JS::Value*) (jsapi.cpp:5726)
==18606==    by 0x419652: JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, char const*, unsigned long, JS::Value*) (jsapi.cpp:5741)
==18606==    by 0x420660: JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, char const*, JS::Value*) (jsapi.cpp:5758)
==18606==    by 0x40835D: Load(JSContext*, unsigned int, JS::Value*) (js.cpp:725)
==18606==    by 0x4966F1: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:372)
==18606==    by 0x48B769: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2454)
==18606==  Address 0x100616f3d8 is not stack'd, malloc'd or (recently) free'd

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: Remove lookupProperty to prevent interpreter reentrance.

This patch use the native inlining method to substitute regexp_exec by regexp_test instead of looking up for the "test" property on the regexp class.

This prevent the execution of the proxy "has" function of the reported test case.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 665241 [details] [diff] [review]
Remove lookupProperty to prevent interpreter reentrance.

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

This is much nicer, thanks!

::: js/src/builtin/RegExp.cpp
@@ +562,5 @@
>  }
>  
> +bool
> +js::ExecuteRegExp(JSContext *cx, RegExpExecType execType, HandleObject regexp,
> +              HandleString string, MutableHandleValue rval)

Nit: fix indentation of the last line.

@@ +601,5 @@
>  
>      /* Step 9a. */
>      if (i < 0 || i > length) {
>          reobj->zeroLastIndex();
> +        rval.address()->setNull();

Nit: rval.setNull();

@@ +613,5 @@
>          return false;
>      }
>  
>      /* Step 11 (with sticky extension). */
> +    if (re->global() || (!rval.address()->isNull() && re->sticky())) {

Nit: rval.isNull()

@@ +614,5 @@
>      }
>  
>      /* Step 11 (with sticky extension). */
> +    if (re->global() || (!rval.address()->isNull() && re->sticky())) {
> +        if (rval.address()->isNull())

Same here.

@@ +640,5 @@
> +    JSString *input = ToString(cx, (args.length() > 0) ? args[0] : UndefinedValue());
> +    if (!input)
> +        return false;
> +
> +    RootedString string(cx, input);

Nit: you can use RootedString directly:

RootedString input(cx, ToString(...));
if (!input)
    return false;

::: js/src/ion/CodeGenerator.cpp
@@ +232,5 @@
> +
> +    Register output = ToRegister(lir->output());
> +    Label notBool, end;
> +    Assembler::Condition isFalse = masm.testBoolean(Assembler::NotEqual, JSReturnOperand);
> +    masm.j(isFalse, &notBool);

Nit: masm.branchTestBoolean(Assembler::NotEqual, JSReturnOperand, &notBool);

::: js/src/ion/LIR-Common.h
@@ +1712,5 @@
> +
> +    LRegExpTest(const LAllocation &regexp, const LAllocation &string)
> +    {
> +        setOperand(0, regexp);
> +        setOperand(0, string);

s/0/1

::: js/src/ion/Lowering.cpp
@@ +1114,5 @@
> +    JS_ASSERT(ins->string()->type() == MIRType_String);
> +
> +    LRegExpTest *lir = new LRegExpTest(useRegisterAtStart(ins->regexp()),
> +                                       useRegisterAtStart(ins->string()));
> +    return define(lir, ins) && assignSafepoint(lir, ins);

s/define/defineVMReturn

::: js/src/ion/MCallOptimize.cpp
@@ +652,5 @@
>  
> +IonBuilder::InliningStatus
> +IonBuilder::inlineRegExpTest(uint32 argc, bool constructing)
> +{
> +    if (argc != 2 || constructing)

Looking at the other natives, this should be argc != 1.

@@ +658,5 @@
> +
> +    if (getInlineArgType(argc, 0) != MIRType_Object)
> +        return InliningStatus_NotInlined;
> +    if (getInlineArgType(argc, 1) != MIRType_String)
> +        return InliningStatus_NotInlined;

We should also check the return type == MIRType_Boolean, or for regexp.exec probably if the type set *contains* MIRType_Boolean since exec will return an object in the interpreter. Can you make sure the micro-benchmark in bug 764743 comment 0 does not regress?

::: js/src/ion/MIR.h
@@ +3001,5 @@
> +    }
> +
> +    AliasSet getAliasSet() const {
> +        // Changing slots of the regexp can change the behaviour of it.
> +        return AliasSet::Load(AliasSet::Slot);

It's probably best to use the default alias set. It shouldn't matter much and RegExp.test can have side-effects:

/abc(def)/.test("abcdef");
assertEq(RegExp.$1, "def");

Comment 5

[Daniel Veditz [:dveditz]]

From some of the signatures and the files the patch touches we're assuming this is ion-only.

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> @@ +658,5 @@
> > +
> > +    if (getInlineArgType(argc, 0) != MIRType_Object)
> > +        return InliningStatus_NotInlined;
> > +    if (getInlineArgType(argc, 1) != MIRType_String)
> > +        return InliningStatus_NotInlined;
> 
> We should also check the return type == MIRType_Boolean, or for regexp.exec
> probably if the type set *contains* MIRType_Boolean since exec will return
> an object in the interpreter.

Ok for checking for regexp.test, but I don't understand how the returned type set of regexp.exec might contain the boolean type.  The return type of regexp.exec is either Null or an Object, even if the result of the call does not escape.

> Can you make sure the micro-benchmark in bug
> 764743 comment 0 does not regress?

./js -b _build/bug764743.js
586
runtime = 586.669 ms

Comment 7

[Nicolas B. Pierron [:nbp]]

This Bug can safely be open once landed because it only affect fx18 and if there is any issue it can be fix easily or disabled.

https://hg.mozilla.org/integration/mozilla-inbound/rev/564d554c4318

Comment 8

[Ed Morley [:emorley]]

Backed out for M2 assertions:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=564d554c4318

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6a9cb1be492

Comment 9

[Nicolas B. Pierron [:nbp]]

CC efaust to help figuring out the Lowering issue of DOM method calls.

Comment 10

[Nicolas B. Pierron [:nbp]]

Attachment: Remove lookupProperty to prevent interpreter reentrance.

- Rebase the patch, still failing on M2.

Comment 11

[Nicolas B. Pierron [:nbp]]

Comment on attachment 665241 [details] [diff] [review]
Remove lookupProperty to prevent interpreter reentrance.

Oops, not so obsolete yet.

Comment 12

[Nicolas B. Pierron [:nbp]]

Comment on attachment 667180 [details] [diff] [review]
Remove lookupProperty to prevent interpreter reentrance.

Oops, bad patch.

Comment 13

[Eric Faust [:efaust]]

The problem causing the M2 failures was improper initialization of targets in jsop_call()

The line 

RootedFunction target(cx, NULL);

should be changed to:

RootedFunction target(cx, numTargets == 1 ? targets[0]->toFunction() : NULL);

This seems to solve at least this part of the problem.

Comment 14

[Nicolas B. Pierron [:nbp]]

https://tbpl.mozilla.org/?tree=Try&rev=32a06e4a3e8c (try without test case)
https://hg.mozilla.org/integration/mozilla-inbound/rev/d41ca12d2527 (with test case)

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d41ca12d2527

Comment 16

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.