892787 - Differential Testing: Different error message involving Function.prototype

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

[0].some(Function.prototype)
function f() {
    new Function.prototype
}
for (x in [0, 0]) {
    try {
        f()
    } catch (e) {
        print(e)
    }
}

shows the following on a 64-bit debug deterministic js shell on m-c rev 8aca531ff163:

TypeError: function () {
} is not a constructor
TypeError: function () {
} is not a constructor

but shows the following with --ion-eager:

TypeError: function () {
} is not a constructor


This seems to be an old bug. Setting needinfo for jandem as this may cloud other correctness bugs.

Comment 1

[Jan de Mooij [:jandem]]

Good catch. The JITs have to check for fun->isInterpretedConstructor() before attaching a JSOP_NEW stub or inlining the call.

Inlining isInterpretedConstructor() requires multiple branches though and is a bit complicated. So before I do that I'd like to try to remove the SELF_HOSTED_CTOR flag and add a INTERPRETED_CTOR flag, which we can easily check from JIT code.

Comment 2

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

jandem, this blocks me finding other differential testing bugs - it might be nice to have this fixed soon.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #2)
> jandem, this blocks me finding other differential testing bugs - it might be
> nice to have this fixed soon.

Sorry for the delay, I hope I can fix this tomorrow. It requires fixing both JITs and the changes are not trivial, that's why I didn't get to it yet..

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

Fixes Ion and Baseline constructing calls to fail if the callee is not a constructor. This adds an extra branch to the generic call path but I think this is okay because (1) we only do this for |new| calls and (2) it's only the polymorphic call path; if we know the callee we don't need the check.

Also adds tests for both the monomorphic and polymorphic cases.

Comment 5

[Hannes Verschore [:h4writer]]

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

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

::: js/src/jit/IonBuilder.cpp
@@ +4968,5 @@
> +        // Don't optimize if we're constructing and the callee is not an
> +        // interpreted constructor, so that CallKnown does not have to
> +        // handle this case (it should always throw).
> +        if (constructing && !target->isInterpretedConstructor())
> +            target = NULL;

This should probably go into makeCallHelper. Since we are calling makeCall in a dozen of places. And I'm pretty sure we can get the same issue there... The only annoying thing I see, is that makeCall also uses the target...

::: js/src/jit/IonMacroAssembler.h
@@ +412,5 @@
> +            // SELF_HOSTED_CTOR is not set. This means the callee must be
> +            // Function.prototype or a self-hosted function that's not a
> +            // constructor.
> +            branchTest32(Assembler::Zero, scratch, Imm32(JSFunction::SELF_HOSTED_CTOR << 16),
> +                         label);

The branching here doesn't agree with the c++ code. If we have a FunctionPrototype that is a selfHostedConstructor we will not branch. I'm not sure if we have that, but it could get added lateron... So that could introduce bugs.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Patch v2

As discussed on IRC, I added an assert to IonBuilder::makeCall.

There's only one Function.prototype and it should never be marked as self-hosted constructor, but I added a debug-only check for that just to be sure.

Comment 7

[Jan de Mooij [:jandem]]

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

Linux Try: https://tbpl.mozilla.org/?tree=Try&rev=5974016d7c49

Nice find, Gary. Sorry it took so long. Please keep them coming!

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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