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

RESOLVED FIXED in Firefox 39

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: arai, Assigned: arai)

Tracking

Trunk
mozilla39
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(13 attachments, 1 obsolete attachment)

6.31 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
1.95 KB, patch
Felipe
: review+
Details | Diff | Splinter Review
2.13 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
1.07 KB, patch
jandem
: review+
Details | Diff | Splinter Review
2.02 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
5.36 KB, patch
peterv
: review+
Details | Diff | Splinter Review
1.49 KB, patch
edgar
: review+
Details | Diff | Splinter Review
1.24 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
2.90 KB, patch
jandem
: review+
Details | Diff | Splinter Review
1.37 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
3.47 KB, patch
jgraham
: review+
Details | Diff | Splinter Review
6.42 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
4.67 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

Comment 1

4 years ago
* 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).
(Assignee)

Comment 2

4 years ago
Created attachment 8564696 [details] [diff] [review]
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+"."
Attachment #8564696 - Flags: review?(gavin.sharp)
(Assignee)

Comment 3

4 years ago
Created attachment 8564697 [details] [diff] [review]
Part 2: Do not use non-standard flag argument of String.prototype.match/replace in browser/components/translation/.
Attachment #8564697 - Flags: review?(felipc)
(Assignee)

Comment 4

4 years ago
Created attachment 8564698 [details] [diff] [review]
Part 3: Do not use non-standard flag argument of String.prototype.replace in browser/metro/.
Attachment #8564698 - Flags: review?(mbrubeck)
(Assignee)

Comment 5

4 years ago
Created attachment 8564699 [details] [diff] [review]
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.
Attachment #8564699 - Flags: review?(jdemooij)
(Assignee)

Comment 6

4 years ago
Created attachment 8564700 [details] [diff] [review]
Part 5: Do not use non-standard flag argument of String.prototype.replace in docshell/test/unit/.
Attachment #8564700 - Flags: review?(gavin.sharp)
(Assignee)

Comment 7

4 years ago
Created attachment 8564701 [details] [diff] [review]
Part 6: Do not use non-standard flag argument of String.prototype.replace in dom/.
Attachment #8564701 - Flags: review?(peterv)
(Assignee)

Comment 8

4 years ago
Created attachment 8564702 [details] [diff] [review]
Part 7: Do not use non-standard flag argument of String.prototype.replace in dom/mobileconnection/gonk/.
Attachment #8564702 - Flags: review?(echen)
(Assignee)

Comment 9

4 years ago
Created attachment 8564703 [details] [diff] [review]
Part 8: Do not use non-standard flag argument of String.prototype.replace in dom/payment/.
Attachment #8564703 - Flags: review?(fabrice)
(Assignee)

Comment 10

4 years ago
Created attachment 8564705 [details] [diff] [review]
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.
Attachment #8564705 - Flags: review?(jdemooij)
(Assignee)

Comment 11

4 years ago
Created attachment 8564706 [details] [diff] [review]
Part 10: Do not use non-standard flag argument of String.prototype.replace in mobile/android/.
Attachment #8564706 - Flags: review?(rnewman)
(Assignee)

Comment 12

4 years ago
Created attachment 8564707 [details] [diff] [review]
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.
Attachment #8564707 - Flags: review?(rnewman)
Attachment #8564703 - Flags: review?(fabrice) → review+
(Assignee)

Comment 13

4 years ago
Created attachment 8564708 [details] [diff] [review]
Part 12: Do not use non-standard flag argument of String.prototype.replace in testing/web-platform/tests/.
Attachment #8564708 - Flags: review?(jgriffin)
(Assignee)

Comment 14

4 years ago
Created attachment 8564709 [details] [diff] [review]
Part 13: Do not use non-standard flag argument of String.prototype.replace in toolkit/.
Attachment #8564709 - Flags: review?(mak77)
(Assignee)

Comment 15

4 years ago
Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6249e23b4657
(reordered patches after the try run)
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Attachment #8564706 - Flags: review?(rnewman) → review+
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>");
Attachment #8564707 - Flags: review?(rnewman) → feedback+
(Assignee)

Comment 17

4 years ago
Created attachment 8564773 [details] [diff] [review]
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.
Attachment #8564707 - Attachment is obsolete: true
Attachment #8564773 - Flags: review?(rnewman)
Attachment #8564773 - Flags: review?(rnewman) → review+
Attachment #8564701 - Flags: review?(peterv) → review+
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("&", "&amp;", "g")
> -             .replace('"', "&quot;", "g")
> -             .replace("'", "&apos;", "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?
Attachment #8564697 - Flags: review?(felipc) → review+
(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 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 = "";
Attachment #8564699 - Flags: review?(jdemooij) → review+
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.
Attachment #8564705 - Flags: review?(jdemooij) → review+

Updated

4 years ago
Attachment #8564702 - Flags: review?(echen) → review+
Summary: Fix in-tree consumers that uses non-standard flag argument of String.prototype.{search,match,replace}. → Fix in-tree consumers that use non-standard flag argument of String.prototype.{search,match,replace}
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.
Attachment #8564708 - Flags: review?(jgriffin) → review?(james)
Attachment #8564708 - Flags: review?(james) → review+
Attachment #8564696 - Flags: review?(gavin.sharp) → review+
Attachment #8564700 - Flags: review?(gavin.sharp) → review+
(Assignee)

Updated

4 years ago
Attachment #8564709 - Flags: review?(mak77) → review?(mano)
Attachment #8564698 - Flags: review?(mbrubeck) → review+
(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?
Flags: needinfo?(ttaubert)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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 :(
Flags: needinfo?(ttaubert)
(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.
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.
(Assignee)

Comment 27

4 years ago
Thank you all, landed Part 1-12.

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=c65ef0c02ed3
Keywords: leave-open
(Assignee)

Updated

4 years ago
Attachment #8564709 - Flags: review?(mano) → review?(gavin.sharp)
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)
Attachment #8564709 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 30

4 years ago
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/8892b6d198c6
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.