1510193 - Differential Testing: Different output message involving TypeError messages

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P3)

Description

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

try {
    Object.seal(this);
    x = (function(stdlib, foreign, heap) {
        "use asm";
        function f() {}
        return f;
    })(this, {}, new ArrayBuffer(4096));
    print(x);
} catch (e) {
    print(e);
}


$ ./js-64-dm-linux-a6996c5491a5 --fuzzing-safe --no-threads --no-baseline --no-ion --no-asmjs testcase.js
ReferenceError: x is not defined

$ ./js-64-dm-linux-a6996c5491a5 --fuzzing-safe --ion-offthread-compile=off --ion-eager testcase.js
TypeError: can't define property "TypedObject": global is not extensible

Tested this on m-c rev a6996c5491a5.

My configure flags are:

AR=ar sh ./configure --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python3 -u -m funfuzz.js.compile_shell -b "--enable-more-deterministic -R ~/trees/mozilla-beta" -r a6996c5491a5

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset:   496959:a6996c5491a5
user:        ffxbld
date:        Mon Oct 15 14:19:22 2018 -0700
summary:     Update configs. IGNORE BROKEN CHANGESETS CLOSED TREE NO BUG a=release ba=release

This is unlikely to be the regressor but I dug some more and found a nearby changeset:

https://hg.mozilla.org/releases/mozilla-beta/rev/7a95803c70dd

Jason, was bug 1488417 properly backed out? This does not seem to affect mozilla-central.

Comment 1

[Jason Orendorff [:jorendorff]]

I'll try to bisect this locally later. Cloning a tree for it now.

I don't think it's related to bug 1488417. It's probably just a dup of something that got fixed in central.

Comment 2

[Jason Orendorff [:jorendorff]]

OK, so first of all, apparently I'm not capable of having two gecko trees for more than about 60 seconds before getting them confused.

If I change `65.0a1` to `65.0` in /config/milestone.txt, re-configure and re-build the shell, I can reproduce this bug even in central. So it seems the bug is *fixed* by some code that's ifdef'd off in beta/release.

Comment 3

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

> ifdef'd off in beta/release.

Which means beta/release are affected but not central?

Comment 4

[Jason Orendorff [:jorendorff]]

Yes, I think that's the case. I'm bisecting now and then maybe we'll see why...

Comment 5

[Jason Orendorff [:jorendorff]]

The first bad revision is:
changeset:   494810:29614e10d512
user:        Lars T Hansen <[e-mail]>
date:        Wed May 09 11:35:47 2018 +0200
summary:     Bug 1459900 - Implement struct.new. r=luke

Oh, it's obvious. TypedObject is disabled in beta/release, but this changeset introduced some new code that can try to enable it regardless. (The Object.seal() is relevant because it catches us trying to define TypedObject later.)

+bool
+Module::makeStructTypeDescrs(JSContext* cx,
+                             MutableHandle<StructTypeDescrVector> structTypeDescrs) const
+{
+    // Not just any prototype object will do, we must have the actual StructTypePrototype.
+    RootedObject typedObjectModule(cx, GlobalObject::getOrCreateTypedObjectModule(cx,
+                                                                                  cx->global()));

If this use of TypedObject is an implementation detail, and it can't expose any of the disabled features to script, then I think GlobalObject::initTypedObjectModule can just skip the DefineDataProperty if TypedObject is disabled.

If it does expose TypedObject to script, we probably need to be more careful, ask for it only if we actually need it, and bail out of asm compilation if it's disabled...?

Comment 6

[Lars T Hansen [:lth]]

I can surely introduce some appropriate ifdeffery here.

Comment 7

[Lars T Hansen [:lth]]

As background, the very experimental wasm GC feature (which is Nightly-only and then only behind flags at the moment) uses TO machinery to represent its types and instances.  In turn, OdinMonkey translates asm.js to wasm and then compiles the wasm, which presumably triggers the creation of the TypedObject property whether the GC feature is used or not, or it seems, whether it is present or not.

Re "visible to script", the GC feature assumes that TypedObject is standard thing, though it doesn't really count on it being available to scripts per se, and doesn't look it up on the global.

Comment 8

[Lars T Hansen [:lth]]

Attachment: bug1510193-dont-touch-the-typed-objects-without-reason.patch

This should be sufficient to ensure that we don't create the TypedObject module lazily and that we don't depend on it if it is not enabled explicitly.

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 9028571 [details] [diff] [review]
bug1510193-dont-touch-the-typed-objects-without-reason.patch

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

OK, cool.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/01648611837d
don't use TypedObjects if not available and necessary. r=jorendorff

Comment 11

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/01648611837d