1318017 - Call GetPrototypeFromConstructor for generator/async function and Intl constructors

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[André Bargull [:anba]]

GetPrototypeFromConstructor needs to be called to ensure subclassing works as expected.

Comment 1

[André Bargull [:anba]]

Attachment: bug1318017.patch

I've added you as the reviewer because this patch and bug 755821 touch the same code in jsfun.cpp, and it also affects the new async function code.

Comment 2

[Tooru Fujisawa [:arai]]

Comment on attachment 8811367 [details] [diff] [review]
bug1318017.patch

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

Thanks.

It would be nice to check subclassed AsyncFunction and GeneratorFunction really works when it's called.
maybe in separated patch.

::: js/src/builtin/Intl.cpp
@@ +807,2 @@
>      if (construct) {
> +        // Step 2 (Inlined 9.1.14, OrdinaryCreateFromConstructor).

Step 5.

@@ +826,4 @@
>      RootedValue locales(cx, args.length() > 0 ? args[0] : UndefinedValue());
>      RootedValue options(cx, args.length() > 1 ? args[1] : UndefinedValue());
>  
> +    // Step 3.

Step 6.

::: js/src/jsfun.cpp
@@ +1735,2 @@
>      RootedObject proto(cx);
> +    if (!isAsync && !GetPrototypeFromCallableConstructor(cx, args, &proto))

Separating `if (...)` for each might be clearer, since the meaning of `false` differs for each.

   if (!isAsync) {
       if (!GetPrototypeFromCallableConstructor(cx, args, &proto))
           return false;
   }

it's up to you tho.

::: js/src/tests/Intl/Collator/construct-newtarget.js
@@ +29,5 @@
> +
> +// Set a different constructor as NewTarget.
> +obj = Reflect.construct(MyCollator, [], Array);
> +assertEq(obj instanceof MyCollator, false);
> +assertEq(obj instanceof Intl.Collator, false);

would be nice to check
  assertEq(obj instanceof Array, true);

same for others.

Comment 3

[André Bargull [:anba]]

Attachment: bug1318017.patch

Updated patch per review comments. Carrying r+ from Arai.


Also fixed a bug from the previous patch:
I was calling CallArgs::callee() after CallArgs::rval() was used, which is not allowed. There was no assertion error ("Assertion failure: !usedRval_") because two separate CallArgs instances were created. I've changed the FunctionConstructor method to use `const CallArgs&` to catch this kind of error.

Comment 4

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6e9025a3d296b4c267b07c7eeb4711493a7ca45

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e8c181309b1
Call GetPrototypeFromConstructor for generator, async function, and Intl constructors. r=arai

Comment 6

[Carsten Book [:Tomcat]]

had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=39434628&repo=mozilla-inbound

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/95e326672620
Backed out changeset 4e8c181309b1 for j1 test failures

Comment 8

[André Bargull [:anba]]

Argh, I forgot to add the has-Intl guard in the reftest line.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4e8c181309b1

Comment 10

[André Bargull [:anba]]

(In reply to Carsten Book [:Tomcat] from comment #9)
> https://hg.mozilla.org/mozilla-central/rev/4e8c181309b1

Now I'm a bit confused, I thought the change was (rightfully) backed out from inbound?

Comment 12

[Phil Ringnalda (:philor)]

The way we do things, including filing an intermittent-failure bug for a failure that isn't intermittent and then three minutes later backing out the cause and then three hours later merging something from between the bustage and the backout to mozilla-central, will become much clearer to you if you buy a pair of shoes from http://www.spearshoes.com/ and walk around in them for a while.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/95e326672620

Comment 14

[André Bargull [:anba]]

Attachment: bug1318017.patch

Updated patch to include Intl guard, carrying r+ from Arai.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e48faa1e4783
Call GetPrototypeFromConstructor for generator, async function, and Intl constructors. r=arai

Comment 16

[Carsten Book [:Tomcat]]

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