Closed Bug 1297117 Opened 5 years ago Closed 5 years ago

Audit use of getUrlFromAboutReader(), consider making private

Categories

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

defect
Not set
normal

Tracking

(firefox51 fixed)

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: ahunt, Assigned: ahunt)

References

Details

Attachments

(4 files)

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.
Assignee: nobody → 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.
(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 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.
Attachment #8787742 - Flags: review-
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?
Attachment #8787739 - Flags: review?(s.kaspari) → review+
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?
Attachment #8787740 - Flags: review?(s.kaspari) → review+
Comment on attachment 8787741 [details]
Bug 1297117 - Add documentation to stripAboutReaderUrl

https://reviewboard.mozilla.org/r/76430/#review74860
Attachment #8787741 - Flags: review?(s.kaspari) → review+
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
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.