Differential Testing: Different output message involving TypeError messages

RESOLVED FIXED in mozilla65

Status

()

defect
P3
major
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: gkw, Assigned: lth)

Tracking

(Blocks 2 bugs, {testcase})

Trunk
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 affected, firefox65 unaffected)

Details

Attachments

(1 attachment)

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.
Flags: needinfo?(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.
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.
> ifdef'd off in beta/release.

Which means beta/release are affected but not central?
Yes, I think that's the case. I'm bisecting now and then maybe we'll see why...
The first bad revision is:
changeset:   494810:29614e10d512
user:        Lars T Hansen <lhansen@mozilla.com>
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...?
Flags: needinfo?(jorendorff) → needinfo?(lhansen)
I can surely introduce some appropriate ifdeffery here.
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Flags: needinfo?(lhansen)
Priority: -- → P3
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.
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.
Attachment #9028571 - Flags: review?(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.
Attachment #9028571 - Flags: review?(jorendorff) → review+
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/01648611837d
don't use TypedObjects if not available and necessary. r=jorendorff
https://hg.mozilla.org/mozilla-central/rev/01648611837d
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.