Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Description • 7 years ago

The following testcase crashes on mozilla-central revision 23a4b7430dd7 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --no-baseline --no-ion):

x = evalcx('lazy');
oomTest(function () {
    x.eval;
});


Backtrace:

#0  0x00000000009d2aa8 in js::GlobalObject::setThrowTypeError (this=0x7f54bcfb1060, fun=0x7f54bcfb57c0) at js/src/vm/GlobalObject.h:150
#1  0x00000000009ae1d5 in CreateFunctionPrototype (cx=0x7f54be271000, key=<optimized out>) at js/src/jsfun.cpp:935
#2  0x0000000000b537b0 in js::GlobalObject::resolveConstructor (cx=cx@entry=0x7f54be271000, global=..., key=key@entry=JSProto_Function) at js/src/vm/GlobalObject.cpp:198
#3  0x0000000000b53fd8 in js::GlobalObject::ensureConstructor (cx=cx@entry=0x7f54be271000, global=..., global@entry=..., key=key@entry=JSProto_Function) at js/src/vm/GlobalObject.cpp:122
#4  0x0000000000594009 in CreateObjectConstructor (cx=0x7f54be271000, key=<optimized out>) at js/src/builtin/Object.cpp:1365
#5  0x0000000000b53802 in js::GlobalObject::resolveConstructor (cx=cx@entry=0x7f54be271000, global=..., key=key@entry=JSProto_Object) at js/src/vm/GlobalObject.cpp:215
#6  0x0000000000b53fd8 in js::GlobalObject::ensureConstructor (cx=cx@entry=0x7f54be271000, global=..., global@entry=..., key=key@entry=JSProto_Object) at js/src/vm/GlobalObject.cpp:122
#7  0x00000000009384fa in JS_ResolveStandardClass (cx=cx@entry=0x7f54be271000, obj=..., id=..., id@entry=..., resolved=resolved@entry=0x7ffe0e7f926f) at js/src/jsapi.cpp:1075
#8  0x0000000000447de5 in sandbox_resolve (cx=0x7f54be271000, obj=..., id=..., resolvedp=0x7ffe0e7f926f) at js/src/shell/js.cpp:3252
#9  0x0000000000b48acf in js::CallResolveOp (recursedp=<synthetic pointer>, propp=..., id=..., obj=..., cx=0x7f54be271000) at js/src/vm/NativeObject-inl.h:559
#10 js::LookupOwnPropertyInline<(js::AllowGC)1> (cx=0x7f54be271000, obj=..., id=..., propp=..., donep=0x7ffe0e7f9357) at js/src/vm/NativeObject-inl.h:652
#11 0x0000000000b57658 in NativeGetPropertyInline<(js::AllowGC)1> (cx=cx@entry=0x7f54be271000, obj=..., receiver=..., id=..., nameLookup=nameLookup@entry=NotNameLookup, vp=...) at js/src/vm/NativeObject.cpp:2099
#12 0x0000000000b58180 in js::NativeGetProperty (cx=cx@entry=0x7f54be271000, obj=..., obj@entry=..., receiver=..., receiver@entry=..., id=..., id@entry=..., vp=..., vp@entry=...) at js/src/vm/NativeObject.cpp:2145
#13 0x0000000000a492dc in js::GetProperty (vp=..., id=..., receiver=..., obj=..., cx=0x7f54be271000) at js/src/vm/NativeObject.h:1442
#14 js::Wrapper::get (this=this@entry=0x1e70720 <js::CrossCompartmentWrapper::singleton>, cx=cx@entry=0x7f54be271000, proxy=..., proxy@entry=..., receiver=receiver@entry=..., id=id@entry=..., vp=vp@entry=...) at js/src/proxy/Wrapper.cpp:143
/snip

For detailed crash information, see attachment.

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Comment 3 • 7 years ago

Probably related to bug 1219128.

Waldo, thoughts?

	Jan de Mooij [:jandem] Assignee
	Comment 5 • 7 years ago

So at least for this instance there's an easy fix: instead of initializing %ThrowTypeError% in CreateFunctionPrototype, we can do this lazily when we need it (for strict-mode arguments.callee, so that's ~never in real code). A small performance improvement, so we want to do this anyway.

It's also a bit simpler because we can now use NewNativeFunction since we no longer have to worry about Function.prototype being initialized.

I tried to write a test with newGlobal instead of evalcx("lazy") but I wasn't able to trigger the assertion failure with that.

	Jan de Mooij [:jandem] Assignee
	Comment 6 • 7 years ago

Comment on attachment 8939834 [details] [diff] [review]
Patch

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

::: js/src/jit-test/tests/basic/bug1348407.js
@@ +1,2 @@
> +x = evalcx("lazy");
> +oomTest(function () {

This needs:

if (!('oomTest' in this))
    quit();

	Jeff Walden [:Waldo]
	Comment 7 • 7 years ago

Comment on attachment 8939834 [details] [diff] [review]
Patch

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

::: js/src/jsfun.h
@@ +796,5 @@
>  extern bool
>  fun_symbolHasInstance(JSContext* cx, unsigned argc, Value* vp);
>  
> +extern void
> +ThrowFunctionOrArgumentsTypeError(JSContext* cx);

This name doesn't do it for me.  Why can't it continue to have the same name?  "function or arguments TypeError" is just really vague for what this is actually doing, IMO.

::: js/src/vm/ArgumentsObject.cpp
@@ +800,3 @@
>          attrs = JSPROP_PERMANENT | JSPROP_GETTER | JSPROP_SETTER;
> +        getter = CastAsGetterOp(throwTypeError);
> +        setter = CastAsSetterOp(throwTypeError);

Eugh, still not a fan of having users cast this stuff rather than the implementation doing it internally.  :-(

::: js/src/vm/GlobalObject.cpp
@@ +350,5 @@
> +    if (!NativeDefineProperty(cx, throwTypeError, nameId, nonConfigurableDesc, nameResult))
> +        return nullptr;
> +    MOZ_ASSERT(nameResult);
> +
> +    global->setSlot(THROWTYPEERROR, ObjectValue(*throwTypeError));

setReservedSlot

	Pulsebot
	Comment 8 • 7 years ago

Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0f8dc928f55
Initialize %ThrowTypeError% lazily, simplify CreateFunctionPrototype. r=jwalden

	Bogdan Tara[:bogdan_tara | bogdant]
	Comment 9 • 7 years ago
bugherder

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