825705 - IonMonkey: Crash [@ JSScript::ensureRanAnalysis] or [@ AnalyzeNewScriptProperties] or [@ js_CreateThisForFunctionWithProto] or "Assertion failure: JS_ObjectIsFunction(0, this)," or "Assertion failure: JS_ObjectIsFunction(__null, this),"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack from Windows 7

evalcx("\
    var x = newGlobal().Object;\
    function f() { return new x; }\
    f();\
    f();\
", newGlobal());

asserts js debug shell on m-c changeset 0bb4773db082 with --ion-eager at Assertion failure: JS_ObjectIsFunction(0, this),

Jesse helped reduce this.

s-s because I had a lot of testcases that show this assertion, but which are difficult to reproduce.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

The testcase in comment 0 is 32-bit only. I may have a 64-bit testcase soon.

(gdb) x/i $pc
=> 0x80cd8f7 <JSScript::ensureRanAnalysis(JSContext*, JS::Handle<JSScript*>)+23>:	mov    0x24(%eax),%ecx
(gdb) x/b $eax
0xffffff87:	Cannot access memory at address 0xffffff87
(gdb) x/b $ecx
0xffffbd0c:	0x87
(gdb)

Inspection shows weird memory address 0xffffff87 being accessed. Assuming sec-critical.

autoBisect is now running.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   116169:7e44aec095e3
user:        Till Schneidereit
date:        Wed Oct 10 22:53:51 2012 +0200
summary:     Bug 784293 - Support creating and lazily cloning arbitrary objects in self-hosted code. r=jwalden

Bug 784293 only landed in Firefox 20, so this does not seem to affect branches.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: partially reduced 64-bit testcase that crashes and asserts in Linux

Please check that the patches fix this testcase as well.

Comment 4

[Jan de Mooij [:jandem]]

I think autobisect is wrong and this is a regression from bug 813773. CreateThis assumes the callee is an object, but here it's a proxy.

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> CreateThis assumes the callee is an object, but here it's a proxy.

Er, it assumes the callee is a *function*.

Comment 6

[Jan de Mooij [:jandem]]

There's another problem with proxies, they can intercept the "get obj.prototype" we use. The following testcase throws an exception with --no-jm but shouldn't throw:

function f() {
    for (var i=0; i<100; i++)
	new O();
}
var O = new Proxy(function() {}, {
    get: function() {
	throw "get trap";
    }
});
f();

Comment 7

[Hannes Verschore [:h4writer]]

Attachment: Don't get prototype when callee is unknown

CreateThis can now get constructed with and without "prototype". For unknown callee we use the one without "prototype". Therefore we are now only getting the callee.prototype when needed (and allowed)

Comment 8

[Hannes Verschore [:h4writer]]

Attachment: Don't get prototype when callee is unknown

Updated as requested on IRC

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 697496 [details] [diff] [review]
Don't get prototype when callee is unknown

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

Looks good, r=me with comments below addressed.

::: js/src/ion/Lowering.cpp
@@ +243,5 @@
>  
>  bool
> +LIRGenerator::visitCreateThisWithProto(MCreateThisWithProto *ins)
> +{
> +    LCreateThisO *lir = new LCreateThisO(useRegisterOrConstantAtStart(ins->getCallee()),

Nit: you can now rename LCreateThisO to LCreateThisWithProto and LCreateThisV to LCreateThis

::: js/src/ion/TypePolicy.h
@@ +175,5 @@
>          return staticAdjustInputs(ins);
>      }
>  };
>  
> +// Combine multiple policies.

You can remove this class since it's unused now.

Comment 10

[Hannes Verschore [:h4writer]]

Comment on attachment 697496 [details] [diff] [review]
Don't get prototype when callee is unknown

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

::: js/src/ion/CodeGenerator.cpp
@@ -2044,5 @@
>  
>  bool
> -CodeGenerator::emitCreateThisVM(LInstruction *lir,
> -                                const LAllocation *proto,
> -                                const LAllocation *callee)

Also remove the class definition of this method in CodeGenerator.h

Comment 11

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/67e44e98555c

Comment 12

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/67e44e98555c

Comment 13

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.