864097 - Use String.startsWith/contains instead of regexp in browser.js

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Comment 1

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch v1

Comment 2

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

try: https://tbpl.mozilla.org/?tree=Try&rev=470323e8f97b

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 740054 [details] [diff] [review]
patch v1

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

This looks fine to me (except for the one comment below).

::: mobile/android/chrome/content/browser.js
@@ +3231,5 @@
>            let tabURL = this.browser.currentURI.specIgnoringRef;
>            if (article == null || (article.url != tabURL)) {
>              // Don't clear the article for about:reader pages since we want to
>              // use the article from the previous page
> +            if (!tabURL.startsWith("about:reader"))

The regex that this replaced is case-insensitive. I don't know if tabURL can be uppercase or not. Please verify either way (and correct if necessary) before landing this.

Comment 4

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch v2

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> 
> ::: mobile/android/chrome/content/browser.js
> @@ +3231,5 @@
> >            let tabURL = this.browser.currentURI.specIgnoringRef;
> >            if (article == null || (article.url != tabURL)) {
> >              // Don't clear the article for about:reader pages since we want to
> >              // use the article from the previous page
> > +            if (!tabURL.startsWith("about:reader"))
> 
> The regex that this replaced is case-insensitive. I don't know if tabURL can
> be uppercase or not. Please verify either way (and correct if necessary)
> before landing this.


Thank you for speedy review!
How about the new patch?

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 740315 [details] [diff] [review]
patch v2

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

::: mobile/android/chrome/content/browser.js
@@ +3034,5 @@
>          // Attach a listener to watch for "click" events bubbling up from error
>          // pages and other similar page. This lets us fix bugs like 401575 which
>          // require error page UI to do privileged things, without letting error
>          // pages have any privilege themselves.
> +        if (target.documentURI.startsWith("about:")) {

I'm having second thoughts about this. If target.documentURI doesn't exist (is undefined or null) then the old code would return false but the new code will throw an error. I don't know if this happens in practice, but I'm not the right person to review this.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 740315 [details] [diff] [review]
patch v2

Looks like nsStandardURL will lowercase the scheme and the host, so we do not need to lowercase "about:xxx" because "about" is the scheme and "xxx" is the host.
Also, nsDocument::GetDocumentURL appears to always return a string, truncated or filled with a URL spec.


>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>-        if (/^about:/.test(target.documentURI)) {
>+        if (target.documentURI.startsWith("about:")) {

We should be a bit more selective here. This block should only be executed for error pages, which should mean:

/^about:(neterror|certerror|blocked)/

I see desktop has a slightly different condition:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4259

Since ErrorPageEventHandler only cares about "about:certerror" and "about:blocked" let's just check from them explicitly:

let docURI = targetdocumentURI;
if (docURI.startsWith("about:certerror") || docURI.startsWith("about:blocked")) {

Also use docURI in the :about:reader" test that's just below the error page test

>-            if (!/^about:reader/i.test(tabURL))
>+            if (!tabURL.toLowerCase().startsWith("about:reader"))

Should be fine to drop the toLowerCase

r+ with those changes

Comment 7

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch v3

Thank you for your comments, Mark.

And just to be safe, I'm confirming this patch on try: https://tbpl.mozilla.org/?tree=Try&rev=9e7e0ce61bb0

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 740790 [details] [diff] [review]
patch v3

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
>+        let docURI = target.documentURI;
>+        if (docURI.startsWith("about:certerror") || docURI.startsWith("about:blocked")) {
>           this.browser.addEventListener("click", ErrorPageEventHandler, true);
>           let listener = function() {
>             this.browser.removeEventListener("click", ErrorPageEventHandler, true);
>             this.browser.removeEventListener("pagehide", listener, true);
>           }.bind(this);
> 
>           this.browser.addEventListener("pagehide", listener, true);
>         }
> 
>-        if (/^about:reader/.test(target.documentURI))
>+        if (target.documentURI.startsWith("about:reader"))

Might as well use "docURI" here instaed of "target.documentURI"

Comment 9

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Mark Finkle (:mfinkle) from comment #8)
> Comment on attachment 740790 [details] [diff] [review]
> patch v3
> 
> >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
> >+        let docURI = target.documentURI;
> >+        if (docURI.startsWith("about:certerror") || docURI.startsWith("about:blocked")) {
> >           this.browser.addEventListener("click", ErrorPageEventHandler, true);
> >           let listener = function() {
> >             this.browser.removeEventListener("click", ErrorPageEventHandler, true);
> >             this.browser.removeEventListener("pagehide", listener, true);
> >           }.bind(this);
> > 
> >           this.browser.addEventListener("pagehide", listener, true);
> >         }
> > 
> >-        if (/^about:reader/.test(target.documentURI))
> >+        if (target.documentURI.startsWith("about:reader"))
> 
> Might as well use "docURI" here instaed of "target.documentURI"

Oops!

Comment 10

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch v3.1

Comment 11

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea697909f4f2

Comment 12

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/ea697909f4f2

Comment 13

[Masatoshi Kimura [:emk]]

(In reply to Mark Finkle (:mfinkle) from comment #6)
> Looks like nsStandardURL will lowercase the scheme and the host, so we do
> not need to lowercase "about:xxx" because "about" is the scheme and "xxx" is
> the host.

nsStandardURL does not handle "about:" URIs. nsSimpleURI does.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Masatoshi Kimura [:emk] from comment #13)
> (In reply to Mark Finkle (:mfinkle) from comment #6)
> > Looks like nsStandardURL will lowercase the scheme and the host, so we do
> > not need to lowercase "about:xxx" because "about" is the scheme and "xxx" is
> > the host.
> 
> nsStandardURL does not handle "about:" URIs. nsSimpleURI does.

Which does seem to lowercase the scheme.

Comment 15

[Masatoshi Kimura [:emk]]

What about the opaque part? "about:Blank" wasn't lowercased.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Masatoshi Kimura [:emk] from comment #15)
> What about the opaque part? "about:Blank" wasn't lowercased.

I'm not too worried about it. Humans don't normally type those. If we find bugs, we'll fix them.