Closed Bug 1244722 Opened 4 years ago Closed 4 years ago

Tab Queue issues with "Draw over other apps" disabled

Categories

(Firefox for Android :: General, defect)

46 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 48
Tracking Status
firefox46 --- wontfix
firefox47 + fixed
firefox48 + verified

People

(Reporter: TeoVermesan, Assigned: sebastian)

References

(Blocks 1 open bug, )

Details

(Keywords: regression)

Attachments

(2 files)

Steps to reproduce:
1. Go to Settings -> Apps -> Draw over other apps -> Aurora/Nightly and disable the option
2. Open Firefox and go to Settings -> General and tap "Open multiple links" option
3. From the "Open multiple links" dialog tap on Settings (to enable 'Draw over other apps'
4. Enable the option
5. Go back

Expected results:
- "Open multiple links" option is enabled in Settings

Actual results: 
- "Open multiple links" dialog is still displayed
- The option is not enabled

Note;
- I am not able to reproduce this every time 
- Please take a look at the following video: https://www.youtube.com/watch?v=CYJaY_Ldn7Q&feature=youtu.be

Also:
- sometimes with "Draw over other apps" disabled, with a clean Aurora profile, go to Settings -> General and turn on "Open multiple links" option => the option is enabled. No dialog to turn on "Draw over other apps" is shown.
- please take a look at the following video: https://youtu.be/eOsRBy8WLrM
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
(In reply to Teodora Vermesan (:TeoVermesan) from comment #0)
> - sometimes with "Draw over other apps" disabled, with a clean Aurora
> profile, go to Settings -> General and turn on "Open multiple links" option
> => the option is enabled. No dialog to turn on "Draw over other apps" is
> shown.
> - please take a look at the following video: https://youtu.be/eOsRBy8WLrM

I have seen this a couple of times today too. This is really strange. ContextCompat.canDrawOverlays() seems to return true while the toggle is actually off. This almost looks like a bug in the Android system. Actually trying to draw over other apps fails in this situation.
Currently I can't get this in an unbroken state again on my device - even re-install, clear data, reboot does not fix this.
This looks a lot like this:
https://code.google.com/p/android/issues/detail?id=198671

If we are hitting this then the return value of canDrawOverlays() is influenced by other Firefox installations on the device (e.g. if Nightly can draw over other apps then we might not show the prompt in Aurora). This needs some more investigation.
(In reply to Sebastian Kaspari (:sebastian) from comment #2)
> If we are hitting this then the return value of canDrawOverlays() is
> influenced by other Firefox installations on the device (e.g. if Nightly can
> draw over other apps then we might not show the prompt in Aurora). This
> needs some more investigation.

Yeah, in my case I could "fix" the issue by enabling or disabling overdraw for all installed Firefox versions.

@Teodora: Do you have multiple Firefox versions installed on the device? Do you still see the issue if all Firefox versions are using the same setting?
Flags: needinfo?(teodora.vermesan)
I have both Nightly and Aurora installed on this device. 
Scenarios:

1. First both versions have "Draw over other apps" set to "No":
-On Aurora I try to enable "Open multiple links" feature: but the dialog is still displayed and the option is not enabled. "Draw over other apps" is set to "Yes".
-On Nightly works ok. "Draw over other apps" is set to "Yes" also.

Now both versions have "Draw over other apps" set to "Yes". I clear data, set the option to "No" and try the same steps. 
-First tried with Nightly and works ok
-Then tried with Aurora: "Open multiple links" option is enabled and no dialog to turn on "Draw over other apps" is shown. "Draw over other apps" remains set to "No" for Aurora and "Yes" to Nightly.

2. Restart the device and repeat the above steps with the other version first:

Both versions have "Draw over other apps" set to "No":
-On Nightly I try to enable "Open multiple links" feature: but the dialog is still displayed and the option is not enabled. "Draw over other apps" is set to "Yes"
-On Aurora works ok. "Draw over other apps" is set to "Yes", also

Now both versions have "Draw over other apps" set to "Yes". I clear data, set the option to "No" and try the same steps. 
-First tried with Aurora and works ok.
-Then tried with Nightly: "Open multiple links" option is enabled and no dialog to turn on "Draw over other apps" is shown. "Draw over other apps" remains set to "No" for Nightly and "Yes" to Aurora
Flags: needinfo?(teodora.vermesan)
Unfortunately it seems like we won't be able to fix that - at least until the Android team fixes this bug:
https://code.google.com/p/android/issues/detail?id=198671

This is caused by us using "sharedUserId" and then Android mixing up the settings. Even though it seems like we do not need "sharedUserId" anymore, we can't get rid of it because this would revoke access to our own files:
https://code.google.com/p/android/issues/detail?id=1227
https://code.google.com/p/android/issues/detail?id=14074

Android never implemented a "downgrade path" for removing or changing a sharedUserId of an existing application.
(In reply to Sebastian Kaspari (:sebastian) from comment #5)
> Unfortunately it seems like we won't be able to fix that - at least until
> the Android team fixes this bug:
> https://code.google.com/p/android/issues/detail?id=198671

There seems to be no movement in this bug at all.

As a last resort we could try to not use Settings.canDrawOverlays() and instead try to add an invisible overlay: If successful -> We can add overlays. Remove it again. If exception: Seems like we do not have permission.
I looked at the code again and added a comment to the bug report:
https://code.google.com/p/android/issues/detail?id=198671#c7

This method is obviously broken for apps that use sharedUserId. I implemented the workaround mentioned in comment 5. This seems to work just fine. I'll test some more and then push it to review.

Another option would be to use reflection to access Settings.isCallingPackageAllowedToDrawOverlays() and provide it with the correct package name. But this is prone to break if this non-public API changes.
Comment on attachment 8736275 [details]
MozReview Request: Bug 1244722 - TabQueueHelper.canDrawOverlays(): Implement workaround for Android bug. r?ahunt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43193/diff/1-2/
Comment on attachment 8736275 [details]
MozReview Request: Bug 1244722 - TabQueueHelper.canDrawOverlays(): Implement workaround for Android bug. r?ahunt

https://reviewboard.mozilla.org/r/43193/#review40105
Attachment #8736275 - Flags: review?(ahunt) → review+
https://hg.mozilla.org/integration/fx-team/rev/a70b50e2830f3316d5a1307bb9f8231ca88efc52
Bug 1244722 - TabQueueHelper.canDrawOverlays(): Implement workaround for Android bug. r=ahunt
https://hg.mozilla.org/mozilla-central/rev/a70b50e2830f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Flags: needinfo?(s.kaspari)
Comment on attachment 8736275 [details]
MozReview Request: Bug 1244722 - TabQueueHelper.canDrawOverlays(): Implement workaround for Android bug. r?ahunt

We should uplift this. I somehow forgot to request this.

Approval Request Comment

[Feature/regressing bug #]: Runtime permissions - Introduced with Firefox 46.0 - Bug 1212830. Maybe it's too late for 46, but requesting uplift anyways.

[User impact if declined]: If multiple versions of Firefox are installed locally (say Release and Beta) then the prompt for "Draw over other apps" might not be shown when it should or shown even if we already have the permission.

[Describe test coverage new/current, TreeHerder]: Local testing with multiple installations.

[Risks and why]: Low - This has been in Nightly for two weeks now.

[String/UUID change made/needed]: -
Flags: needinfo?(s.kaspari)
Attachment #8736275 - Flags: approval-mozilla-beta?
Attachment #8736275 - Flags: approval-mozilla-aurora?
Keywords: regression
I think this can wait till 47. We are very close to the 46 release, this is a very particular case (having more than one version installed) and doesn't reproduce every time.
Attachment #8736275 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Hi Teodora, could you please verify this bug is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(teodora.vermesan)
Comment on attachment 8736275 [details]
MozReview Request: Bug 1244722 - TabQueueHelper.canDrawOverlays(): Implement workaround for Android bug. r?ahunt

Recent regression, was manually verified by dev, Aurora47+
Attachment #8736275 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Based on steps from comment 4:

1. First both versions have "Draw over other apps" set to "No":
-On Aurora: affected
-On Nightly works ok. "Draw over other apps" is set to "Yes". "Tab queue" is enabled. The prompt is dismissed

Now both versions have "Draw over other apps" set to "Yes". I clear data, set the option to "No" and try the same steps. 
-First tried with Nightly and works ok. "Draw over other apps" is set to "Yes". "Tab queue" is enabled. The prompt is dismissed
-Then tried with Aurora: "Open multiple links" option is enabled and no dialog to turn on "Draw over other apps" is shown. "Draw over other apps" remains set to "No" for Aurora and "Yes" to Nightly.

2. Restart the device and repeat the above steps with the other version first:

Both versions have "Draw over other apps" set to "No":
-On Nightly works ok. "Draw over other apps" is set to "Yes". "Tab queue" is enabled. The prompt is dismissed
-On Aurora works ok. "Draw over other apps" is set to "Yes", also

Now both versions have "Draw over other apps" set to "Yes". I clear data, set the option to "No" and try the same steps. 
-First tried with Aurora and works ok. 
-Then tried with Nightly: and works ok. 

3. "Draw over other apps" set to "Yes" for Aurora and "No" for Nightly
-On Nightly works ok.


Verified as fixed using:
Device: Nexus 6 (Android 6.0)
Build: Firefox for Android 48.0a1 (2016-04-17)
Flags: needinfo?(teodora.vermesan)
Modified patch for Aurora (Fixing imports)
Flags: needinfo?(s.kaspari) → needinfo?(cbook)
(In reply to Sebastian Kaspari (:sebastian) from comment #20)
> Created attachment 8744348 [details] [diff] [review]
> 1244722-TQ-Aurora.patch
> 
> Modified patch for Aurora (Fixing imports)

landed by ryan
Flags: needinfo?(cbook)
Verified based on comment 19
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.