Disable non-standard flag argument of String.prototype.{search,match,replace} in non-release build.

RESOLVED FIXED in Firefox 47

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: arai, Assigned: arai)

Tracking

({dev-doc-complete, site-compat})

Trunk
mozilla47
dev-doc-complete, site-compat
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Telemetry added in bug 1142351 for non-standard flag argument of String.prototype.{search,match,replace} is still hitting high both in web content (150k in nightly, 6M in release) and add-on (27k in nightly), after 7 cycles from Firefox 39 that warning was added to.  it's a little decreasing but won't reach acceptable level shortly.

It's not believable that it's used in usual web content, as it's non-standard, and it won't work in all other JS engine.  So, disabling it shouldn't break so much web pages.

As a first step, I'd like to disable it in non-release channel, to see what happens.   maybe, some website/add-on breaks and we get bug report about it.

Some add-ons will break because of this, but disabling only in non-release won't affect so much users, and will help add-on authors fixing the issue.


in bug 887016, I'm implementing ES6 compliant version, that uses totally different code path for flag argument handling.  if this bug gets fixed before bug 887016,  bug 887016 patch should reflect the change.

Updated

2 years ago
Keywords: dev-doc-needed, site-compat
(Assignee)

Comment 1

2 years ago
Created attachment 8716141 [details] [diff] [review]
Disable non-standard flag argument from String.prototype.{search,match,replace} in non-release channel.

Changed match/search/replace to ignore flag argument on non-release build.

in this patch, warning and telemetry are still enabled on non-release build, so that if something stops working, web content/add-on author could notice it's the reason, and we could also see whether this patch reduces the usage or not.

Almost green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&
revision=bc00591ac5de
Assignee: nobody → arai.unmht
Attachment #8716141 - Flags: review?(jdemooij)
Comment on attachment 8716141 [details] [diff] [review]
Disable non-standard flag argument from String.prototype.{search,match,replace} in non-release channel.

Exciting!

It would be good to make this a runtime flag though (so the tests for this can call a function to enable it). That way, if we have to enable it again by default for some reason, we can be confident the tests still pass. It also avoids getting test failures and fuzz bugs after we merge to beta/release.

A global bool + a shell function to set it to true would be sufficient I think. Basically what I did for __noSuchMethod__ here:

https://bug683218.bmoattachments.org/attachment.cgi?id=8660210
Attachment #8716141 - Flags: review?(jdemooij)
(In reply to Jan de Mooij [:jandem] from comment #2)
> A global bool + a shell function to set it to true would be sufficient I
> think. Basically what I did for __noSuchMethod__ here:

Actually that used RuntimeOptions instead of a global bool; that seems a bit nicer.
(Assignee)

Comment 4

2 years ago
Created attachment 8716319 [details] [diff] [review]
Disable non-standard flag argument of String.prototype.{search,match,replace} in non-release channel.

Thank you for reviewing :)
Added matchFlagArgument runtime option and changed impl/test to use it instead.
Attachment #8716141 - Attachment is obsolete: true
Attachment #8716319 - Flags: review?(jdemooij)
Comment on attachment 8716319 [details] [diff] [review]
Disable non-standard flag argument of String.prototype.{search,match,replace} in non-release channel.

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

Great, thanks!

::: js/src/js.msg
@@ +293,5 @@
>  MSG_DEF(JSMSG_NO_CLASS_CONSTRUCTOR,    0, JSEXN_TYPEERR,   "class default constructors not yet implemented")
>  MSG_DEF(JSMSG_NO_EXPORT_NAME,          0, JSEXN_SYNTAXERR, "missing export name")
>  MSG_DEF(JSMSG_NO_IMPORT_NAME,          0, JSEXN_SYNTAXERR, "missing import name")
>  MSG_DEF(JSMSG_NO_VARIABLE_NAME,        0, JSEXN_SYNTAXERR, "missing variable name")
> +MSG_DEF(JSMSG_OBSOLETED_FLAGS_ARG,     0, JSEXN_NONE, "flags argument of String.prototype.{search,match,replace} is obsoleted")

Nit: s/obsoleted/obsolete/

(Maybe "no longer supported" is a bit less ambiguous? People might think obsolete means "it's deprecated but still works".)

::: js/src/jsstr.cpp
@@ +2209,5 @@
> +                opt = ToString<CanGC>(cx, args[optarg]);
> +                if (!opt)
> +                    return false;
> +            } else {
> +                opt = nullptr;

Nit: RootedString(cx) is initialized to nullptr, so this assignment (and below) could be removed. If you think it's clearer to be explicit here, that's fine though.
Attachment #8716319 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 6

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/faf39373fc660fb0416ad1d4ab6786074cd982ac
Bug 1245801 - Disable non-standard flag argument of String.prototype.{search,match,replace} in non-release channel. r=jandem
Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/non-standard-flags-argument-of-string-methods-has-been-disabled-in-non-release-builds/

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/faf39373fc66
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Assignee)

Comment 9

2 years ago
Updated following documents:
https://developer.mozilla.org/en-US/Firefox/Releases/47
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/match
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/search
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replace
Thank you, :arai!
Keywords: dev-doc-needed → dev-doc-complete

Comment 11

a year ago
Hi, I don't understand why I see the affect in Fx46b2, it breaks my extension due to this, and I can not find this commit in https://mxr.mozilla.org/mozilla-beta/. Have any similar commit landed on Fx46?
(Assignee)

Comment 12

a year ago
I cannot think of any other similar change related to flag argument.
Can you file a bug with some testcase?
Flags: needinfo?(yfdyh000)

Comment 13

a year ago
I apologize, the issue is caused due to another commit and fixed, I accidentally mixed them when testing.
Flags: needinfo?(yfdyh000)
You need to log in before you can comment on or make changes to this bug.