1381275 - Wrong TypeError message for static Promise methods

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Oriol Brufau [:Oriol]]

Run this code:

    Promise.resolve.call({}, [])

Since {} is not a constructor, it throws a TypeError:

    TypeError: [] is not a constructor

But the message is wrong, because it's the this value {} which should be a constructor, not the first argument [].

 - Before bug 1170760:
   No error
 - Between bug 1170760 and bug 911216:
   TypeError: Non-constructor value passed to NewPromiseCapability.
 - Between bug 911216 and bug 1289318:
   TypeError: 0 is not a constructor
 - After bug 1289318:
   TypeError: [] is not a constructor

Comment 1

[Oriol Brufau [:Oriol]]

Attachment: promise-error.patch

This seems to do the trick.

Hi Nick, are you a JS reviewer now?

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8886824 [details] [diff] [review]
promise-error.patch

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

Hey Oriol!

Yes, I've been a reviewer for certain corners of SpiderMonkey for a while now. Not usually this corner (:till is usually a good choice for promise related things) but this looks straightforward enough that I feel comfortable reviewing it.

Change looks correct, but since this has a history of going wrong, we should write a quick regression test in js/src/jit-test/tests/promise. Something like what you've written in the bug description:

  let e;
  try {
    Promise.resolve.call({}, [])
  } catch (ee) {
    e = ee;
  }
  assertEq(e.message, "{} is not a constructor");

Do you have try access yet? If not, I can do a try push for the next iteration of the patch, but we should definitely get you try access by now ;)

Comment 3

[Oriol Brufau [:Oriol]]

Attachment: promise-error.patch

Patch with tests.

Yes, I got try access recently:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ffd7f106a00e3addbd98d910bffd3541b34218d
But not sure if I chose the appropriate test suites for this.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Oriol [:Oriol] from comment #3)
> But not sure if I chose the appropriate test suites for this.

Those try flags look pretty good. In general, a lot of the spidermonkey tests automatically get run if you touch relevant directories, which is nice.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8887197 [details] [diff] [review]
promise-error.patch

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

Looks great! Thanks for writing the test!

Go ahead and add the checkin-needed keyword when the try results come in and look good.

Thanks again!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a201b7d3ed02
Report proper object in NewPromiseCapability TypeError. r=fitzgen

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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