Closed Bug 997973 Opened 6 years ago Closed 5 years ago

More in-tree consumers that call typed array constructors without "new"

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: anba, Assigned: bbouvier)

References

Details

Attachments

(6 files, 1 obsolete file)

Some more test cases where typed array constructors are called without `new`. See bug 980962 for the original bug report.

js/src/jit-test/tests/basic/bug578041.js
js/src/jit-test/tests/basic/bug836601.js
js/src/jit-test/tests/basic/bug984766.js
js/src/jit-test/tests/basic/testBug606138.js
js/src/jit-test/tests/basic/testSlowNativeWithNullThis.js
js/src/jit-test/tests/basic/testTypedArrayInit.js
js/src/jit-test/tests/for-of/typedarrays-6.js
Fun.

The right thing to do is probably to make those ctors MOZ_CRASH when called without new and then run the test suite until it passes.
Attached patch bug997973.patch (obsolete) — Splinter Review
Using the method in comment 1 seems to work. However, the affected tests don't test the same things now (especially tests that define a getter which is a TypedArray constructor function) but I haven't seen how to alter the tests without altering what's getting tested.

This patch only affects TypedArray ctors. Should the same treatment be applied to ArrayBuffer?
Attachment #8410578 - Flags: feedback?(bzbarsky)
Comment on attachment 8410578 [details] [diff] [review]
bug997973.patch

ArrayBuffer should get the same thing, yes.

Past that, you probably want feedback from whoever wrote these tests, not me... :(
Attachment #8410578 - Flags: feedback?(bzbarsky) → feedback+
Benjamin, are you planning to actually land this or ask someone on the JS team to look at it?  I'd really like to do bug 980945, but it's blocked on this and I'm afraid now we'll need to do more treewide sweeps, since I bet people added more uses in the last 6 months.  :(
Flags: needinfo?(benj)
Yes, let's move on with this.
Flags: needinfo?(benj)
While the right way to review this should be to consider each test case in its own, and think about whether the things being tested change once we use |new|, it seems really tedious and unnecessary, knowing that we have fuzzers and all. Fortunately, only jit-tests have tests that include only ArrayBuffer and TypedArray used as functions (not a single crash in js reftests!), so that's a pretty low number of tests.

Asking r? to jandem, Lord of the JITs.
Attachment #8410578 - Attachment is obsolete: true
Attachment #8506980 - Flags: review?(jdemooij)
Attachment #8506980 - Flags: review?(jdemooij) → review+
Round two, fixed all the ones that showed up in try build 1, including gaia [0].

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0905932fbafa

[0] https://github.com/mozilla-b2g/gaia/pull/25329
Assignee: nobody → benj
Status: NEW → ASSIGNED
Changing strategy, testing only linux64 and once it's green, all platforms.
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c3d4816ccc71
(In reply to Benjamin Bouvier [:bbouvier] from comment #11)
> Changing strategy, testing only linux64 and once it's green, all platforms.
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c3d4816ccc71

It is green (orange is unrelated).
All platforms again:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=422eb6857ae2
Attached patch Tests in treeSplinter Review
bz, chose you as a good cross-tree reviewer, feel free to bounce. These are only test changes.

Will run another try run in a bit to make sure this hasn't changed since Monday (sigh). Should i also make a patch in this bug to at least warn that AB and TA ctors shouldn't be called without new?
Attachment #8509563 - Flags: review?(bzbarsky)
Comment on attachment 8509563 [details] [diff] [review]
Tests in tree

r=me
Attachment #8509563 - Flags: review?(bzbarsky) → review+
Attached file link to gaia pr
I've been told I needed a system peer review for this.
Attachment #8510936 - Flags: review?(etienne)
Warning people in the first place seems better than 1) doing nothing and having people adding more uses of this without noticing, 2) forbidding it directly.

I didn't know where to put the WarnIfNotConstructing function. Native-inl.h seemed like a good place to start, as it is included in both files, and probably more natives will need this warning in the future. Let me know of any more appropriate place where to put this.
Attachment #8510960 - Flags: review?(jwalden+bmo)
Comment on attachment 8510936 [details] [review]
link to gaia pr

stamp.

Gaia-try didn't pick up your pull request, please rebase/push -f. Thanks!
Attachment #8510936 - Flags: review?(etienne) → review+
Comment on attachment 8510960 [details] [diff] [review]
Warn if an AB or a TA is constructed without new

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

::: js/src/js.msg
@@ +73,5 @@
>  MSG_DEF(JSMSG_BAD_SURROGATE_CHAR,      1, JSEXN_TYPEERR, "bad surrogate character {0}")
>  MSG_DEF(JSMSG_UTF8_CHAR_TOO_LARGE,     1, JSEXN_TYPEERR, "UTF-8 character {0} too large")
>  MSG_DEF(JSMSG_MALFORMED_UTF8_CHAR,     1, JSEXN_TYPEERR, "malformed UTF-8 character sequence at offset {0}")
>  MSG_DEF(JSMSG_WRONG_CONSTRUCTOR,       1, JSEXN_TYPEERR, "wrong constructor called for {0}")
> +MSG_DEF(JSMSG_BUILTIN_CTOR_NO_NEW,     0, JSEXN_NONE, "calling a builtin constructor without new is deprecated and will be forbidden in ES6")

This message is kind of wrong, wrt those builtins that won't have this change made to them.  (There are still some, right?  I think so.  Hard to imagine String(v) can change meaning.)  Not sure how to address that; maybe add another parameter into which "typed array" or "ArrayBuffer" can be slotted, e.g. "...a builtin %s constructor..." might do it.

::: js/src/vm/ArrayBufferObject.cpp
@@ +466,5 @@
>      if (argc > 0 && !ToInt32(cx, args[0], &nbytes))
>          return false;
>  
> +    if (!WarnIfNotConstructing(cx, args))
> +        return false;

This also should go below the |args| declaration.  (And the |nbytes| declaration should move beneath both of these.)

::: js/src/vm/NativeObject-inl.h
@@ +685,5 @@
> +{
> +    if (args.isConstructing())
> +        return true;
> +    return JS_ReportErrorFlagsAndNumber(cx, JSREPORT_WARNING, js_GetErrorMessage, nullptr,
> +                                        JSMSG_BUILTIN_CTOR_NO_NEW);

I wouldn't be surprised if you need to go back, after this lands, and change it to a warn-once-per-global thing, so as not to spam code that ubiquitously fails to use |new|.  This is something I imagine will only affect web code "in the field", and is not something we can easily anticipate.  But let's see if this works to start.

::: js/src/vm/TypedArrayObject.cpp
@@ +402,5 @@
>          if (!obj)
>              return false;
> +
> +        if (!WarnIfNotConstructing(cx, args))
> +            return false;

I think we want to warn immediately, not just if an object could be constructed.  So move this just beneath the |args| declaration.
Attachment #8510960 - Flags: review?(jwalden+bmo) → review+
odd, I am not sure why I posted to this bug; this try push has no relevant info.
Forgot to remove the leave-open flag. There might be other uses of the ctors without new, but now there is a message warning people they shouldn't do that. Let's get things forward.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.