1358946 - Reader mode page is saved as normal bookmark

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Reading List, defect, P1)

Description

[Nevin Chen(Not active on Bugzilla)]

Since the update Firefox no longer adds a page in readerview mode to reading list, it simply adds it as a normal bookmark. 


Source:
https://www.reddit.com/r/firefox/comments/66iqgq/firefox_53_no_longer_saves_reader_view_pages_on/


Previously we use the url about:reader?url= to specify if it's a reader mode page and cache the page content. But there's securty concern to show the about:reader?url= in url so they are striped.

We need another way to find if the Tab is reader mode or not other than the url.

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]: regression of popular workflow

Comment 2

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1358946 - Fix bug when reader mode page is saved as normal bookmark.

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

Comment 3

[Nevin Chen(Not active on Bugzilla)]

Looked into desktop with @evanxd.

Desktop also shows the url without "about:reader?url="
But if you view page info, the url has that.

So I think in DB it's still saved as "about:reader?url=http:/xxxxxxx" but shows the url without "about:reader?url="

I'm gonna test how that works in Fennec.

Maybe just did my last patch did.

Comment 4

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8861636 [details]
Bug 1358946 - Fix bug when reader mode page is saved as normal bookmark.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133628/diff/1-2/

Comment 5

[Nevin Chen(Not active on Bugzilla)]

I set the flag cause it's related to previous security patch.

Comment 6

[Max Liu [:maliu]]

Comment on attachment 8861636 [details]
Bug 1358946 - Fix bug when reader mode page is saved as normal bookmark.

https://reviewboard.mozilla.org/r/133628/#review136774

::: mobile/android/base/java/org/mozilla/gecko/LauncherActivity.java:86
(Diff revision 2)
>          filterFlags(intent);
>  
>          startActivity(intent);
>      }
>  
>      private void dispatchCustomTabsIntent() {

Why is this in dispatch"CustomTabs"Intent not dispatch"Normal"Intent?

If "about:reader" prefix is not allowed from external, would it be better if we strip it in onCreate before any dispatch take place?

The activity "BrowserApp" is declare exported as true, which means other app can still sent URL with "about:reader" prefix directly to BrowserApp and bypass this stirpping. Would it be better if we strip it from "onNewIntent" and "onCreate" of BrowserApp or GeckoApp?

Comment 7

[Julien Cristau [:jcristau]]

reader mode regression, tracking for 54/55.

Comment 8

[Kevin Brosnan [Ex-Mozilla]]

We have a fix for bug 1338867 in release does this really need to be a security bug?

Comment 9

[Nevin Chen(Not active on Bugzilla)]

Attachment: 1358946.patch

The bookmark/cache didn't work because Android still need the url to contain "about:reader?url=" to know this page is in reader mode page. Then handle bookmarking & caching correctly. 

For the intent issue, I just stripe 'about:reader?url=' to avoid misuse of the intent. see bug 1338867

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8862331 [details]
1358946.patch

Review of attachment 8862331 [details]:
-----------------------------------------------------------------

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
@@ +1167,5 @@
>      @Override
>      public void onCreate(Bundle savedInstanceState) {
>          GeckoAppShell.ensureCrashHandling();
>  
> +        IntentHelper.stripeDataUri(getIntent());

stripe -> strip

@@ +2184,1 @@
>          final SafeIntent intent = new SafeIntent(externalIntent);

This should probably be done on a SafeIntent object

Comment 11

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1358946 - Strip about:reader in intent data uri.

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

Comment 12

[Nevin Chen(Not active on Bugzilla)]

(In reply to Sebastian Kaspari (:sebastian) from comment #10)

> stripe -> strip
Done 
> @@ +2184,1 @@
> >          final SafeIntent intent = new SafeIntent(externalIntent);
> 
> This should probably be done on a SafeIntent object
Using SafeIntent is good. But I think it's a little bit implicit cause I'll also override the data uri when user call "getIntent()" in the acivity.

Would you please  teach me how to delete the old patch and use the new patch in mozreview? Since Daniel has remove the flags so I guess I should use mozreview now?

Comment 13

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8862331 [details]
1358946.patch

Oh.. I just found how to make the old patch absolete :)

Comment 14

[Carsten Book [:Tomcat]]

need review first

Comment 15

[Nevin Chen(Not active on Bugzilla)]

Oh... this previously passed review in attachment 8862331 [details]
I just added some fix from comment 10 and submitted again using mozreview.

Comment 16

[:Gijs (he/him)]

Comment on attachment 8864742 [details]
Bug 1358946 - Strip about:reader in intent data uri.

https://reviewboard.mozilla.org/r/136396/#review139774

r- because this reintroduces bug 1338867. Intents are just a convenient way to send stuff to the browser, but it's probably possible to convince users to copy/paste things into the URL bar themselves, which works just fine.

We shouldn't stop stripping the URL inside the browser, either - this doesn't just remove the code added in bug 1338867, but also entirely removes the code that changes the displayed URL. This means that about:reader URLs show up in the URL bar as-is (which I confirmed by testing this patch on my local android device), and they shouldn't.

Comment 17

[:Gijs (he/him)]

Re-reading the comments here: If you rely on the un-stripped URL to make the "is this page in reader mode" determination, you should keep the unchanged URL in addition to the stripped URL, or you should strip the URL from java code before displaying it in the location bar, but this patch does neither of these things.

Comment 18

[Max Liu [:maliu]]

Comment on attachment 8864742 [details]
Bug 1358946 - Strip about:reader in intent data uri.

https://reviewboard.mozilla.org/r/136396/#review139930

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:2183
(Diff revision 1)
>          return bitmap;
>      }
>  
>      @Override
>      protected void onNewIntent(Intent externalIntent) {
> +        IntentHelper.stripDataUri(getIntent());

Do we get the correct intent from "getIntent()"? or the parameter of onNewIntent "externalIntent"?

::: mobile/android/base/java/org/mozilla/gecko/LauncherActivity.java
(Diff revision 1)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  package org.mozilla.gecko;
>  
>  import android.app.Activity;
> -import android.content.Context;

How about keep import to avoid uplift fail?

::: mobile/android/base/java/org/mozilla/gecko/LauncherActivity.java:33
(Diff revision 1)
>      protected void onCreate(Bundle savedInstanceState) {
>          super.onCreate(savedInstanceState);
>  
>          GeckoAppShell.ensureCrashHandling();
>  
> +        IntentHelper.stripDataUri(getIntent());

Instead of injecting stripDataUri to onCreate and onNewIntent, what if we override "getIntent()"?
Will that cause a little performance problem since we strip and parse the url everytime?

So how about we override "setIntent()"? Whenever system invoke for onCreate and also onNewIntent, they will always get the striped one.

::: mobile/android/base/java/org/mozilla/gecko/webapps/WebAppActivity.java
(Diff revision 1)
>  
>  import android.app.ActivityManager;
>  import android.content.Intent;
>  import android.graphics.Bitmap;
>  import android.net.Uri;
> -import android.os.Build;

How about keep import to avoid uplift fail?

::: mobile/android/base/java/org/mozilla/gecko/webapps/WebAppActivity.java:32
(Diff revision 1)
>  
>  import org.mozilla.gecko.AppConstants;
>  import org.mozilla.gecko.EventDispatcher;
>  import org.mozilla.gecko.GeckoApp;
>  import org.mozilla.gecko.GeckoAppShell;
> -import org.mozilla.gecko.GeckoProfile;
> +import org.mozilla.gecko.IntentHelper;

How about keep import to avoid uplift fail?

::: mobile/android/base/java/org/mozilla/gecko/webapps/WebAppActivity.java
(Diff revision 1)
>  import org.mozilla.gecko.Tabs;
>  import org.mozilla.gecko.util.ColorUtil;
>  import org.mozilla.gecko.util.EventCallback;
>  import org.mozilla.gecko.util.FileUtils;
>  import org.mozilla.gecko.util.GeckoBundle;
> -import org.mozilla.gecko.widget.AnchoredPopup;

How about keep import to avoid uplift fail?

::: mobile/android/base/java/org/mozilla/gecko/webapps/WebAppActivity.java:160
(Diff revision 1)
>       * shortcut has set, if not we reload the homescreens url
>       */
>      @Override
>      protected void onNewIntent(Intent externalIntent) {
>  
> +        IntentHelper.stripDataUri(getIntent());

Do we get the correct intent from "getIntent()"? or the parameter of onNewIntent "externalIntent"?

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Nevin Chen [:nechen] from comment #12)
> (In reply to Sebastian Kaspari (:sebastian) from comment #10)
> 
> > stripe -> strip
> Done 
> > @@ +2184,1 @@
> > >          final SafeIntent intent = new SafeIntent(externalIntent);
> > 
> > This should probably be done on a SafeIntent object
> Using SafeIntent is good. But I think it's a little bit implicit cause I'll
> also override the data uri when user call "getIntent()" in the acivity.

We had problems with badly crafted Intents in the past - that's the reason why we have SafeIntent. If you now start to read from the Intent object directly then this might reintroduce those issues.

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8864742 [details]
Bug 1358946 - Strip about:reader in intent data uri.

https://reviewboard.mozilla.org/r/136396/#review140038

Comment 21

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8864742 [details]
Bug 1358946 - Strip about:reader in intent data uri.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136396/diff/1-2/

Comment 22

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8864742 [details]
Bug 1358946 - Strip about:reader in intent data uri.

https://reviewboard.mozilla.org/r/136396/#review139930

> Instead of injecting stripDataUri to onCreate and onNewIntent, what if we override "getIntent()"?
> Will that cause a little performance problem since we strip and parse the url everytime?
> 
> So how about we override "setIntent()"? Whenever system invoke for onCreate and also onNewIntent, they will always get the striped one.

It may be more effecient. But I'd prefer not to overrdie Android default behavior (although..... :P)
SafeIntent is the previous solution we took. So I want to go for that.

Comment 23

[Max Liu [:maliu]]

Comment on attachment 8864742 [details]
Bug 1358946 - Strip about:reader in intent data uri.

https://reviewboard.mozilla.org/r/136396/#review142710

Comment 24

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 47659d899be0 -d 437489a51017: rebasing 396271:47659d899be0 "Bug 1358946 - Strip about:reader in intent data uri. r=maliu" (tip)
merging mobile/android/chrome/content/browser.js
merging mobile/android/geckoview/src/main/java/org/mozilla/gecko/mozglue/SafeIntent.java
warning: conflicts while merging mobile/android/chrome/content/browser.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 25

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 47659d899be0 -d a0c7189d60e3: rebasing 396286:47659d899be0 "Bug 1358946 - Strip about:reader in intent data uri. r=maliu" (tip)
merging mobile/android/chrome/content/browser.js
merging mobile/android/geckoview/src/main/java/org/mozilla/gecko/mozglue/SafeIntent.java
warning: conflicts while merging mobile/android/chrome/content/browser.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 26

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Looks like this fix is not landed, so not uplifted yet?

Comment 27

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8864742 [details]
Bug 1358946 - Strip about:reader in intent data uri.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136396/diff/2-3/

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/86af05828caf
Strip about:reader in intent data uri. r=maliu

Comment 29

[Nevin Chen(Not active on Bugzilla)]

Hi Wesley 
Sorry for the late. Please request SV for nightly test before uplift this. Thanks

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for Android bustage in SafeIntent.java:

https://hg.mozilla.org/integration/autoland/rev/75760878c7da78938b7ecc3060875f4b2f4ac732

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=86af05828caffe4a0a7cd2a33e1e4e0d54e39b08&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=100085569&repo=autoland

[task 2017-05-18T11:49:26.515046Z] 11:49:26     INFO -  /home/worker/workspace/build/src/mobile/android/base/resources/drawable-xxhdpi/orange_check.png: libpng warning: iCCP: Not recognizing known sRGB profile that has been edited
[task 2017-05-18T11:49:27.143444Z] 11:49:27     INFO -  Note: Some input files use or override a deprecated API.
[task 2017-05-18T11:49:27.143735Z] 11:49:27     INFO -  Note: Recompile with -Xlint:deprecation for details.
[task 2017-05-18T11:49:27.143840Z] 11:49:27     INFO -  /home/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/gecko/mozglue/SafeIntent.java:15: error: package org.mozilla.gecko.util does not exist
[task 2017-05-18T11:49:27.144588Z] 11:49:27     INFO -  import org.mozilla.gecko.util.StringUtils;
[task 2017-05-18T11:49:27.145671Z] 11:49:27     INFO -                               ^
[task 2017-05-18T11:49:27.145741Z] 11:49:27     INFO -  /home/worker/workspace/build/src/java_home/bin/jar cMf gecko-thirdparty.jar -C gecko-thirdparty-classes .
[task 2017-05-18T11:49:27.388044Z] 11:49:27     INFO -  /home/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/gecko/mozglue/SafeIntent.java:144: error: cannot find symbol
[task 2017-05-18T11:49:27.388132Z] 11:49:27     INFO -                  final String strippedUrl = StringUtils.getQueryParameter(url, "url");
[task 2017-05-18T11:49:27.388167Z] 11:49:27     INFO -                                             ^
[task 2017-05-18T11:49:27.388189Z] 11:49:27     INFO -    symbol:   variable StringUtils
[task 2017-05-18T11:49:27.388205Z] 11:49:27     INFO -    location: class SafeIntent
[task 2017-05-18T11:49:27.425041Z] 11:49:27     INFO -  2 errors
[task 2017-05-18T11:49:27.431684Z] 11:49:27     INFO -  /home/worker/workspace/build/src/config/makefiles/java-build.mk:149: recipe for target 'gecko-mozglue.jar' failed
[task 2017-05-18T11:49:27.431928Z] 11:49:27     INFO -  gmake[5]: *** [gecko-mozglue.jar] Error 1

Comment 31

[Nevin Chen(Not active on Bugzilla)]

Attachment: 1358946.patch

Comment 32

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8869297 [details] [diff] [review]
1358946.patch

org.mozilla.gecko.util.StringUtils can't be found at compile time. I'm not sure if mach build causes this. Strange thing is it works on my machine. And I've pulled the latest code from m-c... anyway, the new approach I took will avoid this issue.

Comment 33

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

FW the needinfo to SV team and qe-verify+
This fix is targeted to go to 54 (beta).

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/04554768a8e0
Strip about:reader in intent data uri. r=maliu

Comment 35

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

https://hg.mozilla.org/mozilla-central/rev/04554768a8e0

Comment 36

[Nevin Chen(Not active on Bugzilla)]

Since this bug targest 54. I marked it as 54+.
I'll make a uplift request after qe-verify+.
This is a small change, just remove the cause and add a little check on the incoming intent.

Comment 37

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8869297 [details] [diff] [review]
1358946.patch

Approval Request Comment
[Feature/Bug causing the regression]: Strip the URL in JS side to avoid unexpected URL from external intents
[User impact if declined]: Reader mode pages were saved as normal bookmarks
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: no
[Why is the change risky/not risky?]: All behavior is the same as before(52). We only add an extra check for external intent here.
[String changes made/needed]: no

Comment 38

[Sorina Florean [:sflorean]]

Verified as fixed in build 55.0a1 from 2017/05/31.
Device: Nexus 9 (Android 7.1.1) and Huawei Honor (Android 5.1.1).
Testing was based on adding articles on reading list, prompt displayed, remove articles, reading list folder, number of items.

Comment 40

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8869297 [details] [diff] [review]
1358946.patch

The fix seems low risk and has been verified/tested by SoftVision on Nightly, Beta54+

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e11b668e8294

Comment 42

[Ioana Chiorean]

Taking my NI as Sorina worked on this.

Comment 43

[Ioana Chiorean]

Ni on Sorina to be tested on next Bet a( Tuesday)

Comment 44

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

firefox:54+ is clear, so I guess we'll simplify tracking:fennec flags.

Comment 45

[u549602]

Verified as fixed on the latest Beta build, 54.0b14.
This issue was tested on a Nexus 9 - Android 6.0 and on a Samsung Galaxy S6 EDGE - Android 6.0
Clearing Sorina's NI

Comment 46

[Sorina Florean [:sflorean]]

Removing qe-verify flag and marking 54:verified based on comment 45.