[LeanPlum] - Save as PDF infinite loop

RESOLVED INVALID

Status

()

P2
normal
RESOLVED INVALID
a year ago
a year ago

People

(Reporter: BogdanS, Assigned: cnevinchen)

Tracking

(Blocks: 1 bug)

55 Branch
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox55 affected)

Details

(Whiteboard: [LP_M1])

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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?
(Assignee)

Comment 1

a year ago
Could you please share me the page to reproduce this bug? Thanks!
Flags: needinfo?(bogdan.surd)
(Reporter)

Comment 2

a year ago
(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
Flags: needinfo?(bogdan.surd)
(Assignee)

Comment 3

a year ago
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.
(Assignee)

Updated

a year ago
Assignee: nobody → cnevinchen
Comment hidden (mozreview-request)
(Assignee)

Comment 5

a year ago
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

a year ago
mozreview-review
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

a year ago
mozreview-review
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.
Attachment #8873354 - Flags: review?(s.kaspari) → review-
(Assignee)

Comment 8

a year ago
Hi Joe
Please kindly give some input since this screenshot feature is not reliable (per comment 6)
Thanks!
Flags: needinfo?(jcheng)
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
Flags: needinfo?(jcheng)
Priority: -- → P2
(Assignee)

Comment 10

a year ago
Disabled this message on leanplum server side per product manager's decision.
(Assignee)

Comment 11

a year ago
Since it's a Leanplum backend settng, we can just disable this message and close this bug
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → INVALID

Updated

a year ago
Whiteboard: [LP_M2]

Updated

a year ago
Whiteboard: [LP_M2] → [LP_M1]
You need to log in before you can comment on or make changes to this bug.