1297117 - Audit use of getUrlFromAboutReader(), consider making private

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Andrzej Hunt :ahunt]

ReaderModeUtils.getUrlFromAboutReader() can return null if the input URL is unparseable (i.e.. if someone types their own about:reader URL, e.g. of the form "about:reader;http://I.am.a.broken/url" instead of "about:reader?url=http://I.am.a.usable/url").

We should audit all uses of getUrlFromAboutReader, and possibly use stripAboutReaderUrl (which returns the original URL if the URL is invalid) instead.

Specific cases where this has caused issues:
Bug 1292903 - crash when invalid URL is typed
Bug 1290014 - new favicon code crashes if history contains the broken about:reader;foo URL

In both of these cases we've had to switch to stripAboutReaderUrl, that might just be the safest option going forward, but we should probably check each case individually.

Comment 1

[Andrzej Hunt :ahunt]

One thing I've noticed:
- getAboutReaderForUrl(String) encodes the input URL.
- stripAboutReaderUrl(String) DOES NOT decode the URL.

I wonder if that has anything to do with potential cache clearing failures in Bug 1298347 ?

I'm going to dig deeper into this as part of this bug. It's possible this would also affect favicon fetching in Bug 1300066.

Comment 2

[Andrzej Hunt :ahunt]

Attachment: Bug 1297117 - Replace unnecessary usage of getUrlFromAboutReader with stripAboutReaderUrl

In some of these cases we're duplicating the work of stripAboutReaderUrl. In the other cases
there is no effective difference, however switching to stripAboutReaderUrl allows us
to make getUrlFromAboutReader private, which should help prevent future errors.

Review commit: https://reviewboard.mozilla.org/r/76426/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76426/

Comment 3

[Andrzej Hunt :ahunt]

Attachment: Bug 1297117 - Make getUrlFromAboutReader private to avoid erronous usage

getUrlFromAboutReader can return null. There have been crashes caused by not checking this
result in the past. stripAboutReaderFromUrl is a safer version which returns the input URL
if necessary, and is probably what should be used in new code, hence we can make this method
private.

Review commit: https://reviewboard.mozilla.org/r/76428/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76428/

Comment 4

[Andrzej Hunt :ahunt]

Attachment: Bug 1297117 - Add documentation to stripAboutReaderUrl

Review commit: https://reviewboard.mozilla.org/r/76430/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76430/

Comment 5

[Andrzej Hunt :ahunt]

Attachment: Bug 1297117 - WIP: Decode original URL when parsing about:reader URL

This doesn't seem to have any immediate adverse effects, but I still need
to learn about what differences this might cause in other parts of the code
(favicon handling? bookamrking?).

Review commit: https://reviewboard.mozilla.org/r/76432/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76432/

Comment 6

[Andrzej Hunt :ahunt]

(In reply to Andrzej Hunt :ahunt from comment #1)
> One thing I've noticed:
> - getAboutReaderForUrl(String) encodes the input URL.
> - stripAboutReaderUrl(String) DOES NOT decode the URL.
> 
> I wonder if that has anything to do with potential cache clearing failures
> in Bug 1298347 ?
> 
> I'm going to dig deeper into this as part of this bug. It's possible this
> would also affect favicon fetching in Bug 1300066.

It turns out that getUrlFromAboutReader uses StringUtils.getQueryParameter() which decodes the URL, which means my last commit is completely redundant if not even a bad idea.

Comment 7

[Andrzej Hunt :ahunt]

Comment on attachment 8787742 [details]
Bug 1297117 - WIP: Decode original URL when parsing about:reader URL

https://reviewboard.mozilla.org/r/76432/#review74500

We don't need this, see comments in the bug.

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8787739 [details]
Bug 1297117 - Replace unnecessary usage of getUrlFromAboutReader with stripAboutReaderUrl

https://reviewboard.mozilla.org/r/76426/#review74856

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:750
(Diff revision 1)
>      public Tab getFirstReaderTabForUrl(String url, boolean isPrivate) {
>          if (url == null) {
>              return null;
>          }
>  
>          if (AboutPages.isAboutReader(url)) {

With using stripAboutReader() we can remove the isAboutReader() check here, right?

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8787740 [details]
Bug 1297117 - Make getUrlFromAboutReader private to avoid erronous usage

https://reviewboard.mozilla.org/r/76428/#review74858

::: mobile/android/base/java/org/mozilla/gecko/reader/ReaderModeUtils.java:23
(Diff revision 1)
>       *
>       * @see #stripAboutReaderUrl(String) for a safer version that returns the original URL for malformed/invalid
>       *     URLs.
>       * @return <code>null</code> if the URL is malformed or doesn't contain a URL parameter.
>       */
> -    public static String getUrlFromAboutReader(String aboutReaderUrl) {
> +    private static String getUrlFromAboutReader(String aboutReaderUrl) {

Maybe we should just inline the functionality now?

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8787741 [details]
Bug 1297117 - Add documentation to stripAboutReaderUrl

https://reviewboard.mozilla.org/r/76430/#review74860

Comment 11

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/aeff16129d5a8c9b84dc38e399c33cd1bbceba2e
Bug 1297117 - Replace unnecessary usage of getUrlFromAboutReader with stripAboutReaderUrl r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/fa498a368de326969bcd2c87cae1452522f8513d
Bug 1297117 - Make getUrlFromAboutReader private to avoid erronous usage r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/b9de26cf8806eaf7a35b7210fb117e07e479154d
Bug 1297117 - Add documentation to stripAboutReaderUrl r=sebastian

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/aeff16129d5a
https://hg.mozilla.org/mozilla-central/rev/fa498a368de3
https://hg.mozilla.org/mozilla-central/rev/b9de26cf8806