Wrong TypeError message for static Promise methods

RESOLVED FIXED in Firefox 56

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: Oriol, Assigned: Oriol)

Tracking

unspecified
mozilla56
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 months ago
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
(Assignee)

Comment 1

5 months ago
Created attachment 8886824 [details] [diff] [review]
promise-error.patch

This seems to do the trick.

Hi Nick, are you a JS reviewer now?
Assignee: nobody → oriol-bugzilla
Status: NEW → ASSIGNED
Attachment #8886824 - Flags: review?(nfitzgerald)
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 ;)
Attachment #8886824 - Flags: review?(nfitzgerald)
(Assignee)

Comment 3

5 months ago
Created attachment 8887197 [details] [diff] [review]
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.
Attachment #8886824 - Attachment is obsolete: true
Attachment #8887197 - Flags: review?(nfitzgerald)
(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 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!
Attachment #8887197 - Flags: review?(nfitzgerald) → review+
(Assignee)

Updated

5 months ago
Keywords: checkin-needed

Comment 6

5 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a201b7d3ed02
Report proper object in NewPromiseCapability TypeError. r=fitzgen
Keywords: checkin-needed

Comment 7

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a201b7d3ed02
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox56: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.