Opened 5 months ago

Closed 4 months ago

Last modified 4 days ago

#26826 closed task (fixed)

TBA - Does the app need the SYSTEM_ALERT_WINDOW permission?

Reported by: sysrqb Owned by: tbb-team
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-mobile, TorBrowserTeam201808R
Cc: igt0 Actual Points:
Parent ID: #24796 Points:
Reviewer: Sponsor: Sponsor8

Description

I don't know. We should decide.

Child Tickets

Change History (7)

comment:1 Changed 4 months ago by sysrqb

This is used by the TabQueue subsystem. Quoting the TabQueueService description:

The SYSTEM_ALERT_WINDOW permission is used to allow us to insert a View from
this Service which responds to user interaction, whilst still allowing whatever
is in the background to be seen and interacted with.

So, this is used when Fennec's Tab queue is enabled (false by default). If a user enables Tab queue in Fennec's Settings->General menu, then they are prompted for allowing the SYSTEM_ALERT_WINDOW permission at run-time. If the user grants this permission, then whenever the user chooses opening a link from another app in Fennec/Tor Browser, that link is queued in the tab queue. This additional permission is required because the UI presented in the other app when the user performs this action is a custom toast popup created by the browser. This toast popup is translucent and appears over the origin activity.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1130368 and a mockup https://bug1130368.bmoattachments.org/attachment.cgi?id=8560439

and see https://bugzilla.mozilla.org/show_bug.cgi?id=1130368#c4 and c#5 for more context. But the reasoning was:

> Why use a Service here rather than an Activity? If we're the foreground
> Activity, we can draw over the other application if we don't take up the
> full-screen (think share overlay [1]). That way we don't need to take the
> SYSTEM_ALERT_WINDOW. Services are typically reserved for background work
> (i.e. non-drawing).

This is the main difference between this and the share overlay is that this will
allow the user to ignore the toast and carry on using the application in the
background.

I tried a fair few approaches on this before opting to go down the current route.
The issue with using an activity to display the toast is that although we can set
the background of the acitvity to invisible so the user can see the application
behind it, that application is now in a background state which means that it
doesn't respond to touch events and there's no way that I can route touch events
from our activity to the application behind.  This method is the same as FB us
to do their chatheads (http://stackoverflow.com/questions/15975988/what-apis-in-android-is-facebook-using-to-create-chat-heads).

In this context, the share overlay was implemented in Bug 1044947 - mock https://bug1044947.bmoattachments.org/attachment.cgi?id=8480149

comment:2 Changed 4 months ago by sysrqb

With this said, I'm more in favor of deleting the SYSTEM_ALERT_WINDOW permission. The tab queue is not enable by default, and this is only a pretty UX feature. I like pretty UX features, don't get me wrong, but this seems excessive and I don't think this is something we want in a private browser.

comment:3 Changed 4 months ago by sysrqb

Keywords: TorBrowserTeam201808R added
Status: newneeds_review

I'm open to discussing this, but I created a branch for deleting this. This patch permanently disables (sets selectable=false) the tab queue, as well - the tab queue should not be enabled if the app doesn't have the SYSTEM_ALERT_WINDOW permission. We can delete the Settings entry for the tab queue, as well - if that is preferred.

Branch 26826.

For reference:
https://developer.android.com/reference/android/Manifest.permission.html#SYSTEM_ALERT_WINDOW

comment:4 in reply to:  3 Changed 4 months ago by sysrqb

Replying to sysrqb:

Branch 26826.

If we use this, then it might be a good idea to change the description, too.

<!ENTITY pref_tab_queue_summary4 "Save links until the next time you open &brandShortName;">

Which becomes Save links until the next time you open Tor Browser. This may be confusing for Firefox for Android users who enable tab queue in that app, and they want to enable Tab Queue in Tor Browser but the setting is disabled. Should we change the summary to This feature is not supported in Tor Browser or something like that?

https://gitweb.torproject.org/tor-browser.git/tree/mobile/android/base/locales/en-US/android_strings.dtd?h=tor-browser-60.1.0esr-8.0-1#n531

comment:5 Changed 4 months ago by igt0

Can we create flag something like disable-system-alert? I think it is easier to uplift.

Other good argument about removing it, it is because the saved URL is stored in disk.

comment:6 in reply to:  5 Changed 4 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Replying to igt0:

Can we create flag something like disable-system-alert? I think it is easier to uplift.

We probably could. I went with sysrqb's patch for now as this seems easiest. If we want to take upstream concerns into account the best would be to open a Mozilla ticket with the patch that is working for us attached and ask for feedback in which direction we should go.

If we think it is worthwhile to get it "right" on our side (first) let's think about that in a new bug.

tor-browser-60.1.0esr-8.0-1 (commit de2e1fbe5b024957ed469063bf38b8b274b4f4ba) has the fix.

comment:7 Changed 4 days ago by gk

Sponsor: Sponsor8

Sponsor8 in August 2018.

Note: See TracTickets for help on using tickets.