Closed Bug 1245801 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(1 file, 1 obsolete file)

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.
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.
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+
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
https://hg.mozilla.org/mozilla-central/rev/faf39373fc66
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Thank you, :arai!
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?
I cannot think of any other similar change related to flag argument.
Can you file a bug with some testcase?
Flags: needinfo?(yfdyh000)
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.