1131107 - Fix in-tree consumers that use non-standard flag argument of String.prototype.{search,match,replace}

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

Before fixing bug 1108382, we need to replace all of them with regexp literal or new RegExp(...).

Comment 1

[Tooru Fujisawa [:arai]]

* String.prototype.match/search

They treat string in 1st argument as a RegExp pattern.
  http://people.mozilla.org/~jorendorff/es6-draft.html#sec-string.prototype.match
  http://people.mozilla.org/~jorendorff/es6-draft.html#sec-string.prototype.search
So string literal could be rewritten by regexp literal, and variable could be rewritten by `new RegExp(variable, flags)`.


* String.prototype.replace

It treats string in 1st argument just as a string.
  http://people.mozilla.org/~jorendorff/es6-draft.html#sec-string.prototype.replace
So string literal could be rewritten by regexp literal, with escaping every special characters, but variable could be rewritten by `new RegExp(variable, flags)` only if the value does not contain any special character, and in other case, we need to write an equivalent function (it happens in services/healthreport/healthreporter.jsm).

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Do not use non-standard flag argument of String.prototype.match/replace in browser/.

in browser/components/sessionstore/SessionStore.jsm,
> -    let hosts = Object.keys(cookieHosts).join("|").replace("\\.", "\\.", "g");
> +    let hosts = Object.keys(cookieHosts).join("|").replace(/\./g, "\\.");

i guess this is a pre-existing bug, it does not replace anything, but it should replace "." with BACKSLASH+"."

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Do not use non-standard flag argument of String.prototype.match/replace in browser/components/translation/.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Do not use non-standard flag argument of String.prototype.replace in browser/metro/.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Do not use non-standard flag argument of String.prototype.replace in build/pgo/.

2 more same files exist in js/src.
The value of `k` does not contain any special character.

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Do not use non-standard flag argument of String.prototype.replace in docshell/test/unit/.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Part 6: Do not use non-standard flag argument of String.prototype.replace in dom/.

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Part 7: Do not use non-standard flag argument of String.prototype.replace in dom/mobileconnection/gonk/.

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Part 8: Do not use non-standard flag argument of String.prototype.replace in dom/payment/.

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Part 9: Do not use non-standard flag argument of String.prototype.replace in js/src/.

Replaced consumers which does not test the flag argument itself.
Others should be fixed in bug 1108382.

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Part 10: Do not use non-standard flag argument of String.prototype.replace in mobile/android/.

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Part 11: Do not use non-standard flag argument of String.prototype.replace in services/.

in services/healthreport/healthreporter.jsm,
> -        recordMessage = recordMessage.replace(uri.spec, '<' + thing + 'URI>', 'g');
> -      recordMessage = recordMessage.replace(path, '<' + thing + 'Path>', 'g');
They cannot be rewritten directly, because those value may contain special character. So I added replaceAll function, which replaces all substrings.

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Part 12: Do not use non-standard flag argument of String.prototype.replace in testing/web-platform/tests/.

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Part 13: Do not use non-standard flag argument of String.prototype.replace in toolkit/.

Comment 15

[Tooru Fujisawa [:arai]]

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6249e23b4657
(reordered patches after the try run)

Comment 16

[Richard Newman [:rnewman]]

Comment on attachment 8564707 [details] [diff] [review]
Part 11: Do not use non-standard flag argument of String.prototype.replace in services/.

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

::: services/common/utils.js
@@ +87,3 @@
>  
>      if (!pad) {
> +      s = s.replace(/=/g, "");

Padding can only occur at the end of the string. Now we're using a regex, let's be more explicit about that (and potentially get a faster replace), as well as an early return:

  return s.replace(/=+$/, "");

::: services/healthreport/healthreporter.jsm
@@ +785,4 @@
>      function replace(uri, path, thing) {
>        // Try is because .spec can throw on invalid URI.
>        try {
> +        recordMessage = replaceAll(recordMessage, uri.spec, '<' + thing + 'URI>');

Rather than implementing replaceAll, why not escape the URI and construct a valid regex?

http://stackoverflow.com/questions/3561493/is-there-a-regexp-escape-function-in-javascript/3561711#3561711

Extra points for filing a bug to add a method to RegExp -- "give me a regex that matches this literal string with these flags"!

Here's how I'd do it:

  // Return a /g regex that matches the provided string exactly.
  function regexify(s) {
    return new RegExp(s.replace(/[-\/\\^$*+?.()|[\]{}]/g, "\\$&"), "g");
  }

  try {
    recordMessage = recordMessage.replace(regexify(uri.spec), "<" + thing + "URI>");
  } catch (ex) { }

  recordMessage = recordMessage.replace(regexify(path), "<" + thing + "Path>");

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: Part 11: Do not use non-standard flag argument of String.prototype.replace in services/.

Thank you for your feedback!

(In reply to Richard Newman [:rnewman] from comment #16)
> Comment on attachment 8564707 [details] [diff] [review]
> Part 11: Do not use non-standard flag argument of String.prototype.replace
> in services/.
> 
> Review of attachment 8564707 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: services/common/utils.js
> @@ +87,3 @@
> >  
> >      if (!pad) {
> > +      s = s.replace(/=/g, "");
> 
> Padding can only occur at the end of the string. Now we're using a regex,
> let's be more explicit about that (and potentially get a faster replace), as
> well as an early return:
> 
>   return s.replace(/=+$/, "");

Fixed.

> ::: services/healthreport/healthreporter.jsm
> @@ +785,4 @@
> >      function replace(uri, path, thing) {
> >        // Try is because .spec can throw on invalid URI.
> >        try {
> > +        recordMessage = replaceAll(recordMessage, uri.spec, '<' + thing + 'URI>');
> 
> Rather than implementing replaceAll, why not escape the URI and construct a
> valid regex?
> 
> http://stackoverflow.com/questions/3561493/is-there-a-regexp-escape-function-
> in-javascript/3561711#3561711
> 
> Extra points for filing a bug to add a method to RegExp -- "give me a regex
> that matches this literal string with these flags"!
> 
> Here's how I'd do it:
> 
>   // Return a /g regex that matches the provided string exactly.
>   function regexify(s) {
>     return new RegExp(s.replace(/[-\/\\^$*+?.()|[\]{}]/g, "\\$&"), "g");
>   }
> 
>   try {
>     recordMessage = recordMessage.replace(regexify(uri.spec), "<" + thing +
> "URI>");
>   } catch (ex) { }
> 
>   recordMessage = recordMessage.replace(regexify(path), "<" + thing +
> "Path>");

Yeah, that should be nice :)
Replaced with the regexify function. Also, removed slash from the pattern in it, since it's a special character only in regexp literal, iiuc.

Comment 18

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8564697 [details] [diff] [review]
Part 2: Do not use non-standard flag argument of String.prototype.match/replace in browser/components/translation/.

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

::: browser/components/translation/BingTranslator.jsm
@@ -425,5 @@
> -             .replace("&", "&", "g")
> -             .replace('"', """, "g")
> -             .replace("'", "'", "g")
> -             .replace("<", "&lt;", "g")
> -             .replace(">", "&gt;", "g");

I remember having chosen this form for it being the fastest alternative among the possible ways of doing this. But I don't recall now if the difference was significant enough. Is there any newer analysis of the speed differences between the non-standard String.replace and the one using regexp?

Comment 19

[Till Schneidereit [:till]]

(In reply to :Felipe Gomes from comment #18)
> I remember having chosen this form for it being the fastest alternative
> among the possible ways of doing this.

Since bug 977966 landed, the fastest way of doing this is probably .split('foo').join('bar').

Comment 20

[Jan de Mooij [:jandem]]

Comment on attachment 8564699 [details] [diff] [review]
Part 4: Do not use non-standard flag argument of String.prototype.replace in build/pgo/.

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

This was fixed in Sunspider, so I'd prefer the fix below. r=me with that.

::: build/pgo/js-input/sunspider/regexp-dna.html
@@ +1747,5 @@
>      dnaOutputString += seqs[i].source + " " + (dnaInput.match(seqs[i]) || []).length + "\n";
>   // match returns null if no matches, so replace with empty
>  
>  for(k in subs)
> + dnaInput = dnaInput.replace(new RegExp(k, "g"), subs[k])

I just diffed this file against Sunspider 1.0.2 regexp-dna.js, and they just removed the "g" argument:

- dnaInput = dnaInput.replace(k, subs[k], "g")
+ dnaInput = dnaInput.replace(k, subs[k]) // FIXME: Would like this to be a global substitution in a future version of SunSpider.

And also an unrelated change but please fix while you're here:

-var dnaOutputString;
+var dnaOutputString = "";

Comment 21

[Jan de Mooij [:jandem]]

Comment on attachment 8564705 [details] [diff] [review]
Part 9: Do not use non-standard flag argument of String.prototype.replace in js/src/.

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

r=me with comment below addressed.

::: js/src/devtools/jint/sunspider/regexp-dna.js
@@ -1710,5 @@
>   // match returns null if no matches, so replace with empty
>  
>    /* BEGIN LOOP */
>  for(k in subs)
> - dnaInput = dnaInput.replace(k, subs[k], "g")

Heh, I didn't know we had this js/src/devtools/jint directory. Filed bug 1133574 to remove it.

::: js/src/jit-test/tests/sunspider/check-regexp-dna.js
@@ -1707,5 @@
>      dnaOutputString += seqs[i].source + " " + (dnaInput.match(seqs[i]) || []).length + "\n";
>   // match returns null if no matches, so replace with empty
>  
>  for(k in subs)
> - dnaInput = dnaInput.replace(k, subs[k], "g")

As with the other patch, please just remove the "g" argument here to match newer Sunspider versions.

Comment 22

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8564708 [details] [diff] [review]
Part 12: Do not use non-standard flag argument of String.prototype.replace in testing/web-platform/tests/.

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

Passing to the correct reviewer.

Comment 23

[:Gavin Sharp [email: [e-mail]]]

(In reply to Tooru Fujisawa [:arai] from comment #2)
> in browser/components/sessionstore/SessionStore.jsm,
> > -    let hosts = Object.keys(cookieHosts).join("|").replace("\\.", "\\.", "g");
> > +    let hosts = Object.keys(cookieHosts).join("|").replace(/\./g, "\\.");
> 
> i guess this is a pre-existing bug, it does not replace anything, but it
> should replace "." with BACKSLASH+"."

Missed this comment. I thought the intent would have been to replace "\." with "\\.". Tim, do you know what's up here, or if there are related bugs about this?

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #23)
> (In reply to Tooru Fujisawa [:arai] from comment #2)
> > in browser/components/sessionstore/SessionStore.jsm,
> > > -    let hosts = Object.keys(cookieHosts).join("|").replace("\\.", "\\.", "g");
> > > +    let hosts = Object.keys(cookieHosts).join("|").replace(/\./g, "\\.");
> > 
> > i guess this is a pre-existing bug, it does not replace anything, but it
> > should replace "." with BACKSLASH+"."
> 
> Missed this comment. I thought the intent would have been to replace "\."
> with "\\.". Tim, do you know what's up here, or if there are related bugs
> about this?

If only we had something like RegExp.quote() or .escape()...

Both versions actually do the same thing in that they replace www.example.com with www\.example\.com to create a custom regular expression out of a string.

The former version used a string to create a regexp and thus has to do "\\" do get a single backslash. The new version uses a /regexp/ and only needs a single backslash to escape the ".".

I played around with that regex and we seem to unfortunately not have any test coverage :(

Comment 25

[Richard Newman [:rnewman]]

(In reply to Tim Taubert [:ttaubert] from comment #24)

> If only we had something like RegExp.quote() or .escape()...

See Comment 17. arai already added a oneliner to do it.

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

Ok, so Gavin let me know that actually the regex didn't do anything before. We have no tests for that and it looks like no one ever noticed, which is probably because _splitCookiesFromWindow() is only used for deferred restoration - where about:home is shown first and you can opt into restoring the last session. Cookies are also not super-critical, you might just have to re-login.

We should file a follow-up to fix that and write a test. Or maybe even remove it if we don't need it.

Comment 27

[Tooru Fujisawa [:arai]]

Thank you all, landed Part 1-12.

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=c65ef0c02ed3

Comment 28

[Tooru Fujisawa [:arai]]

Bug number in commit message was wrong, sorry!

https://hg.mozilla.org/mozilla-central/rev/fe24d1b2b608
https://hg.mozilla.org/mozilla-central/rev/a1ece54a978e
https://hg.mozilla.org/mozilla-central/rev/d3df8a46f721
https://hg.mozilla.org/mozilla-central/rev/a4de9f7957d8
https://hg.mozilla.org/mozilla-central/rev/2a7d96c1b3c6
https://hg.mozilla.org/mozilla-central/rev/6619751c0bb5
https://hg.mozilla.org/mozilla-central/rev/ea8d2ec9cf94
https://hg.mozilla.org/mozilla-central/rev/f2b747522714
https://hg.mozilla.org/mozilla-central/rev/523e280701e8
https://hg.mozilla.org/mozilla-central/rev/3115299a072a
https://hg.mozilla.org/mozilla-central/rev/36d46ffdc3a0
https://hg.mozilla.org/mozilla-central/rev/c65ef0c02ed3

Comment 29

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8564709 [details] [diff] [review]
Part 13: Do not use non-standard flag argument of String.prototype.replace in toolkit/.

>diff --git a/toolkit/components/places/BookmarkHTMLUtils.jsm b/toolkit/components/places/BookmarkHTMLUtils.jsm

> function escapeUrl(aText) {
>-  return (aText || "").replace("\"", "%22", "g");
>+  return (aText || "").replace(/\"/g, "%22");

Does |"| really need to be escaped in a regexp literal? Seems to work fine as /"/

(this comment also applies to escapeHtmlEntities)

Comment 30

[Tooru Fujisawa [:arai]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #29)
> Comment on attachment 8564709 [details] [diff] [review]
> Part 13: Do not use non-standard flag argument of String.prototype.replace
> in toolkit/.
> 
> >diff --git a/toolkit/components/places/BookmarkHTMLUtils.jsm b/toolkit/components/places/BookmarkHTMLUtils.jsm
> 
> > function escapeUrl(aText) {
> >-  return (aText || "").replace("\"", "%22", "g");
> >+  return (aText || "").replace(/\"/g, "%22");
> 
> Does |"| really need to be escaped in a regexp literal? Seems to work fine
> as /"/
> 
> (this comment also applies to escapeHtmlEntities)

Thank you for reviewing!
Yes, they're optional, and I removed backslashes and landed

https://hg.mozilla.org/integration/mozilla-inbound/rev/8892b6d198c6

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8892b6d198c6