1489594 - File picker doesn't appear when runtime permissions prompt appears first

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jan Henning [:JanH]]

+++ This bug was initially created as a clone of Bug #1489532 +++

I.e. for me, the file picker works fine if permissions are automatically granted or denied by the OS, but not if a permission prompt pops up.

Triggered by https://hg.mozilla.org/integration/autoland/rev/9f1acf3452e1 of bug 1437382.

The problem seems to be that moving our application-background handling forward to onSaveInstanceState also means that the current activity is cleared earlier from the GeckoActivityMonitor and we run into a problem at https://dxr.mozilla.org/mozilla-central/rev/c2e3be6a1dd352b969a45f0b85e87674e24ad284/mobile/android/base/java/org/mozilla/gecko/FilePicker.java#287-290.

Comment 1

[Jan Henning [:JanH]]

Attachment: Bug 1489594 - Decouple application-background event from clearing of current activity. r?jchen

When the OS needs to display a permission prompt, our current activity is paused
and onSaveInstanceState gets called, however the activity isn't stopped yet.
When the permission handling then returns with the results to us, we display
the file picker using the current activity as retrieved from the GeckoActivity-
Monitor.

The problem is that in bug 1437382, our application-background handling was
changed such that it would already be triggered by the preparatory onSave-
InstanceState call. This had the side effect that the current activity would
be cleared from the GeckoActivityMonitor at that point already. Therefore, in
our case showing the file picker would fail because the GAM would have cleared
the current activity already after the runtime permission prompt caused our
previous activity to save its state.

To fix this, we change the behaviour of the GeckoActivityMonitor such that it
will continue to trigger our application-background handling during onSave-
InstanceState if possible, but will only clear the current activity when it is
stopping for real.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 9007338 [details]
Bug 1489594 - Decouple application-background event from clearing of current activity. r?jchen

Jim Chen [:jchen] [:darchons] has approved the revision.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/910902e3dea7
Decouple application-background event from clearing of current activity. r=jchen

Comment 4

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/910902e3dea7

Comment 5

[Julien Cristau [:jcristau]]

Why did you request release tracking for 62?

Comment 6

[Jan Henning [:JanH]]

For someone to decide whether we want to fix this as a potential ride-along if we do a dot release, or wait until 63.

Comment 7

[Julien Cristau [:jcristau]]

If this dates back to 60 and nobody noticed until now it doesn't seem that severe?

Comment 8

[Jan Henning [:JanH]]

True, I guess.

Comment 9

[Jan Henning [:JanH]]

Comment on attachment 9007338 [details]
Bug 1489594 - Decouple application-background event from clearing of current activity. r?jchen

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1437382
[User impact if declined]: User cannot upload files if this triggers an OS runtime permissions prompt because camera/storage permissions for Firefox haven't yet been granted/permanently denied.
[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?]: Low.
[Why is the change risky/not risky?]: Small change to restore the behaviour from before bug 1437382 as far as retrieving the current activity from the GeckoActivityMonitor is concerned.
[String changes made/needed]: none

Comment 10

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9007338 [details]
Bug 1489594 - Decouple application-background event from clearing of current activity. r?jchen

We are still early in the beta cycle  so taking this for uplift in 63 beta.

Comment 11

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9007338 [details]
Bug 1489594 - Decouple application-background event from clearing of current activity. r?jchen

Regression fix for fennec, uplift accepted for the next beta.

Comment 12

[Pascal Chevrel:pascalc (PTO until April 26)]

(In reply to Pascal Chevrel:pascalc from comment #11)
> Comment on attachment 9007338 [details]
> Bug 1489594 - Decouple application-background event from clearing of current
> activity. r?jchen
> 
> Regression fix for fennec, uplift accepted for the next beta.

it seems I really want this one in :)

Comment 13

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9114c6ecda72

Comment 14

[Stefan Deiac]

Verified as fixed in the latest Nightly 64.0a1 (2018-09-17) and Beta 63.0b7 on Nokia 6 (Android 7.1.1) and LG G4 (Android 5.1).
URL tested: https://uploadfiles.io and https://files.fm