Add console warnings for non-standard flag argument of String.prototype.{search,match,replace}.

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: arai, Assigned: arai)

Tracking

({dev-doc-complete})

Trunk
mozilla39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [DocArea=JS])

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

4 years ago
Derived from bug 1108382.

Now almost all in-tree consumers of flag argument of String.prototype.{search,match,replace} are fixed.
Add deprecation warning for it, in the same way as other non-standard features.

Adding also telemetry might be needed.
Whiteboard: [DocArea=JS]
Assignee

Comment 1

4 years ago
To make sure it's safe to remove flags argument, added telemetry for it, as Part 1.
Is there any other thing required to add telemetry item?

Almost green on try runs:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=992de148a534
    (forgot to add shell-only comment)
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9f4dd0003d3
    (fixed above)
Attachment #8579973 - Flags: review?(jdemooij)
Comment on attachment 8579973 [details] [diff] [review]
Part 1: Collect telemetry about deprecated flag argument for String.prototype.{search,match,replace}.

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

Nice.
Attachment #8579973 - Flags: review?(jdemooij) → review+
Comment on attachment 8579974 [details] [diff] [review]
Part 2: Warn about deprecated flag argument for String.prototype.{search,match,replace}.

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

::: js/src/jsstr.cpp
@@ +2137,5 @@
>                  cx->compartment()->addTelemetry(filename, JSCompartment::DeprecatedFlagsArgument);
>              }
>  
> +            if (!JS_ReportErrorFlagsAndNumber(cx, JSREPORT_WARNING, GetErrorMessage, nullptr,
> +                                              JSMSG_DEPRECATED_FLAGS_ARG))

Reporting a warning each time the flags argument is used will slow us down a lot and will spam the browser's console.

It'd be good to warn at most once per compartment/global. You can do something like I did in bug 1140428, or use GlobalObject::warnOnceAbout (vm/GlobalObject.h)
Attachment #8579974 - Flags: review?(jdemooij)
Assignee

Comment 5

4 years ago
Thank you for reviewing :D
Added `warnedAboutFlagsArgument` member to compartment.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7de970d52299
Attachment #8579974 - Attachment is obsolete: true
Attachment #8581029 - Flags: review?(jdemooij)
Comment on attachment 8581029 [details] [diff] [review]
Part 2: Warn about deprecated flag argument for String.prototype.{search,match,replace}.

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

::: js/src/js.msg
@@ +231,5 @@
>  MSG_DEF(JSMSG_CURLY_IN_COMPOUND,       0, JSEXN_SYNTAXERR, "missing } in compound statement")
>  MSG_DEF(JSMSG_DECLARATION_AFTER_EXPORT,0, JSEXN_SYNTAXERR, "missing declaration after 'export' keyword")
>  MSG_DEF(JSMSG_DECLARATION_AFTER_IMPORT,0, JSEXN_SYNTAXERR, "missing declaration after 'import' keyword")
>  MSG_DEF(JSMSG_DEPRECATED_DELETE_OPERAND, 0, JSEXN_SYNTAXERR, "applying the 'delete' operator to an unqualified name is deprecated")
> +MSG_DEF(JSMSG_DEPRECATED_FLAGS_ARG,    0, JSEXN_NONE, "flags argument of String.prototype.{search,match,replace} are deprecated")

Nit: s/are deprecated/is deprecated/

::: js/src/tests/js1_5/String/replace-flags.js
@@ +8,5 @@
> +
> +options("werror");
> +assertEq(evaluate("'aaaA'.match('a', 'i')", {catchTermination: true}), "terminated");
> +assertEq(evaluate("'aaaA'.search('a', 'i')", {catchTermination: true}), "terminated");
> +assertEq(evaluate("'aaaA'.replace('a', 'b', 'g')", {catchTermination: true}), "terminated");

Does this test still pass, if we warn only once per compartment? If not you can probably do newGlobal().evaluate(...); it'll create a new compartment.
Attachment #8581029 - Flags: review?(jdemooij) → review+
Assignee

Comment 7

4 years ago
Thank you for reviewing :)

(In reply to Jan de Mooij [:jandem] from comment #6)
> ::: js/src/tests/js1_5/String/replace-flags.js
> @@ +8,5 @@
> > +
> > +options("werror");
> > +assertEq(evaluate("'aaaA'.match('a', 'i')", {catchTermination: true}), "terminated");
> > +assertEq(evaluate("'aaaA'.search('a', 'i')", {catchTermination: true}), "terminated");
> > +assertEq(evaluate("'aaaA'.replace('a', 'b', 'g')", {catchTermination: true}), "terminated");
> 
> Does this test still pass, if we warn only once per compartment? If not you
> can probably do newGlobal().evaluate(...); it'll create a new compartment.

Yes, because JS_ReportErrorFlagsAndNumber returns false after `options("werror")`, and `cx->compartment()->warnedAboutFlagsArgument = true;` is not evaluated in that case.
is it better to do `cx->compartment()->warnedAboutFlagsArgument = true` before calling JS_ReportErrorFlagsAndNumber ?
Flags: needinfo?(jdemooij)
(In reply to Tooru Fujisawa [:arai] from comment #7)
> Yes, because JS_ReportErrorFlagsAndNumber returns false after
> `options("werror")`, and `cx->compartment()->warnedAboutFlagsArgument =
> true;` is not evaluated in that case.

Ah, right.

> is it better to do `cx->compartment()->warnedAboutFlagsArgument = true`
> before calling JS_ReportErrorFlagsAndNumber ?

Since this only happens with the non-standard werror option, I don't think it matters too much. Either way is fine with me.
Flags: needinfo?(jdemooij)
Assignee

Updated

4 years ago
Assignee: nobody → arai.unmht
https://hg.mozilla.org/mozilla-central/rev/a7556816cb5f
https://hg.mozilla.org/mozilla-central/rev/d802bf89d877
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Thanks arai! Let's not further advertise this, now that it will be gone at some point. The notes in the compat section are enough imo.
Assignee

Updated

3 years ago
See Also: → 1245801
You need to log in before you can comment on or make changes to this bug.