1368971 - [LeanPlum] - Save as PDF infinite loop

Status: RESOLVED INVALID

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Bogdan Surd, QA [:bsurd, NI]]

Device: 
 - Nexus 5 (Android 6.0.1);
 - Huawei Honor 5X (Android 5.1.1);
Build: 
 - Nightly 55.0a1 (2017-05-31);

Steps to reproduce:
 1. Go to any webpage and take a screenshot.
 2. When the Save as PDF trigger occurs tap on "TRY IT!".

Expected result:
 The page is saved as a PDF, and the trigger is dismissed. 

Actual result:
 The page is saved as a PDF, but the trigger reappears. If the user taps on "TRY IT!" again another PDF is downloaded and the trigger reappears again. 

Notes:
 - The Save as PDF is triggered even when the user saves a page from Settings>Page>Save as PDF
 - The trigger also happens when the user takes a screenshot in the Top Sites, Bookmarks and History panels. This should not happen! <-Log a separate bug?

Comment 1

[Nevin Chen(Not active on Bugzilla)]

Could you please share me the page to reproduce this bug? Thanks!

Comment 2

[Bogdan Surd, QA [:bsurd, NI]]

(In reply to Nevin Chen [:nechen] from comment #1)
> Could you please share me the page to reproduce this bug? Thanks!

Hello Nevin, as mentioned above in the bug description any webpage. Here is a video with the issue reproducing in the Top Sites menu. When I first noticed the issue I was on the facebook.com page. 
 
Video: https://goo.gl/jPY7pF

Comment 3

[Nevin Chen(Not active on Bugzilla)]

It worked on my Pixel L (7.1.2) but id didn't work on my Nexus 5(5.1.1)
I'll dig in more.

Comment 4

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1368971 - Restrict the scope for monitoring Screenshot folder to avoid infinite loop when saving as PDF

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

Comment 5

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8873354 [details]
Bug 1368971 - Restrict the scope for monitoring Screenshot folder to avoid infinite loop when saving as PDF


Turns out that 
1. When we take a screen shot, we monitor MediaStore.Images.Media.EXTERNAL_CONTENT_URI
2. We query via ContentResolver, and get the new screenshot's name , and send telemetry (Event.SHARE Method.BUTTON "screenshot")
3. Within Telemetry, the leanplum event triggers, and display a dialog
4. The user click YES on the dialog, and we start to downalod the pdf
5. Once pdf is downloaded, MediaStore.Images.Media.EXTERNAL_CONTENT_URI got notified there are new files added.
6. We check the ContentObserver, since the MediaStore is not updated , the newest file is till the screenshot. Thus goto (1) and cause infinte loop.

In newer devices (ex. Android 7.1.2) this is not happening.

There are two solution about this
1. sendBoradcast and refresh MediaStore when pdf downloads completed.
2. Only monitor the screenshot folder.

I'll go for approach 2 since it's more clear and we won't get unwanted update when non-screenshot is added to external content Uri. Besides, sendBroadcast is not guaranteed, so I don't want to rely on that.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8873354 [details]
Bug 1368971 - Restrict the scope for monitoring Screenshot folder to avoid infinite loop when saving as PDF

https://reviewboard.mozilla.org/r/144804/#review148714

Note that this screenshotting code is problematic and that's why we disabled all features based on it. (There should be bugs filed):

* It triggers way too often -> Even when no screenshot was taken -> Sometimes multiple times for a single screenshot
* It requires disk reading access. With Runtime Permissions this is something we might not have for a long time (or ever).
* It depends on specific file names ("screenshot") or now folders ("Screenshots") being used. This was always just a guess - we do not know if it's reliable?

Are we sure we want to build something on top of /this/ code again?

::: mobile/android/base/java/org/mozilla/gecko/ScreenshotObserver.java:110
(Diff revision 1)
> -    }
>  
> -    private class MediaObserver extends ContentObserver {
> -        public MediaObserver() {
> -            super(null);
> +    private class MediaObserver extends FileObserver {
> +
> +        MediaObserver() {
> +            super(SCREENSHOT_FOLDER, FileObserver.CREATE);

I guess my question here is: Is this assumption (Folder: Screenshots) true for all devices or just the one you use?

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8873354 [details]
Bug 1368971 - Restrict the scope for monitoring Screenshot folder to avoid infinite loop when saving as PDF

https://reviewboard.mozilla.org/r/144804/#review148726

Please re-request if you want to go ahead with this change and you are sure that the folder approach works for all/most devices.

Comment 8

[Nevin Chen(Not active on Bugzilla)]

Hi Joe
Please kindly give some input since this screenshot feature is not reliable (per comment 6)
Thanks!

Comment 9

[Joe Cheng [:jcheng] (please needinfo)]

given how the in app message is displayed in the initial implementation (unless we can make the HTML5 message work), we should not promote this use case. making this is P2

Comment 10

[Nevin Chen(Not active on Bugzilla)]

Disabled this message on leanplum server side per product manager's decision.

Comment 11

[Nevin Chen(Not active on Bugzilla)]

Since it's a Leanplum backend settng, we can just disable this message and close this bug