1634952 - Firefox Focus on Android: Clearing browsing data stops working from notification

Status: RESOLVED FIXED

(Focus :: General, defect)

Description

[jesse.luoto]

Attachment: index.html

Description

Firefox Focus can be set to a state where browser history cannot be erased before the app is force-stopped. This happens after pressing the "Erase browsing history" notification.

Steps to reproduce

1. Open Firefox Focus
2. Navigate to any webpage
3. Without exiting the app, press the Firefox Focus notification that says "Erase browsing history"

Expected results

Firefox Focus should close the current webpages, erase browsing history and continue operating normally

Actual results

• Firefox Focus closes the current webpages, but does not erase browsing history (cookies, locsalstorage)
• Pressing the trash can icon ("erase" button) does not erase browsing history anymore
• Pressing "Open and erase" from notification does not erase browsing history anymore
• Closing the app via Android's "recent apps" does not erase browsing history anymore

Visit page from proof-on-concept to validate that browsing history is not erased.

Environment tested

• Tested with following apps:
  -- Firefox Focus version 8.2.0 (Build #340992215 🦎 75.0-20200403170909)
  -- Firefox Focus custom built from Github source code, master at dde1484c / tag v8.2.0 (Build #11 🦎 75.0-20200403170909)
• Android 10, security patch 2020-03-01
• Samsung Galaxy S10E

Proof of concept

Serve the attached file index.html from a web server, or visit the url https://output.jsbin.com/mejolejimo (contains the same javascript code).

Open the webpage with Firefox Focus and follow "steps to reproduce". The tracking IDs will not get cleared between erases if steps are followed.

Comment 1

[:Gijs (he/him)]

Stefan, who's best placed to investigate this?

Comment 2

[jesse.luoto]

Just tested and confirmed this using Pixel 3 emulator with Android Q.

Built the master branch with Android Studio.

Comment 3

[jesse.luoto]

Note about the previous test URL:
Seems that the old test URL has expired. You'll need to click the "output" tab or use this link: https://jsbin.com/mejolejimo/1/edit?output
(or serve the index.html from the attachment)

Comment 4

[jesse.luoto]

I noticed that there's a new version of Focus (8.3.0) merged to master three days ago. I just re-tested and verified that this issue still occurs on 8.3.0 version.

Tested with:
Pixel 3 emulator / Firefox Focus 8.3.0 🦎 76.0-20200429185419

Comment 5

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This bug is part of a group of bugs in a security or private group which have the old default Severity of normal which has not been changed, and the default priority of --. This indicates that this bugs Severity should be set to -- so it will show up in triage lists.

Comment 6

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Trying to set that severity again.

Comment 7

[:Gijs (he/him)]

:snorp, can you help triage? Thank you!

Comment 8

[jesse.luoto]

Any news from the triage yet? I'd like to point out that per my own testing this should affect all current Android versions of Firefox Focus.

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

Whoops, I don't think this has actually been triaged. Seems pretty bad.

Dylan are you still maintaining Focus right now? I think we probably need to use a session context id in conjunction with private mode?

Comment 10

[Dylan Roeh (:droeh) (he/him)]

(In reply to James Willcox (:snorp) ([e-mail]) (he/him) from comment #9)

Whoops, I don't think this has actually been triaged. Seems pretty bad.

Dylan are you still maintaining Focus right now? I think we probably need to use a session context id in conjunction with private mode?

To the extent that anyone is. I can write up a patch to generate a unique contextId whenever a new session is created, but I don't think I really agree that that's the correct solution here even if it works -- as far as I can tell the steps described should be deleting everything regardless of contextId.

Comment 11

[Dylan Roeh (:droeh) (he/him)]

It may be a moot point, however, as I can't reproduce this either with a local build or a build from the play store.

I've put up the proof of concept here for convenience (the jsbin link in the bug has hit its limit apparently): https://mammoth-lofty-jodhpur.glitch.me/

• Starting from a fresh install of focus, I load it and see that there are no dirty cookies (of course) and make note of the randomly generated IDs on the page.
• Refreshing the page will complain that there are dirty cookies and comparing will of course show that the randomly generated IDs on the page are the same as they were in the previous step.
• Hitting the trash can button closes all open tabs.
• Navigating back to the proof of concept, it will no longer complain about dirty cookies and will now have new randomly generated IDs.

I've tested this with a local x86-64 debug build on Android Q and with a google play store build (8.3.0) on a Pixel 2 also running Android Q.

Comment 12

[jesse.luoto]

Please use the notification, not the trash can image.

After clicking the notification the trash can button stops working too.

Comment 13

[jesse.luoto]

The bug is only reproducible using the following method:

• Without exiting the app, press the Firefox Focus notification that says "Erase browsing history"

Before pressing the notification, these methods work as expected:

• Pressing the trash can button
• Pressing "Erase and open" from the notification

Comment 14

[Dylan Roeh (:droeh) (he/him)]

(In reply to jesse.luoto from comment #12)

Please use the notification, not the trash can image.

After clicking the notification the trash can button stops working too.

Ah! Sorry about that, misread. I can indeed reproduce now, will investigate further.

Comment 15

[Dylan Roeh (:droeh) (he/him)]

Setting a random contextId does indeed fix the issue, though I'm unconvinced it's the right approach. I'll dig into what our notification is doing a bit before putting this up for review.

Comment 16

[Dylan Roeh (:droeh) (he/him)]

(In reply to Dylan Roeh (:droeh) (he/him) from comment #15)

Setting a random contextId does indeed fix the issue, though I'm unconvinced it's the right approach. I'll dig into what our notification is doing a bit before putting this up for review.

Yeah, to followup here: the problem is that we are failing to get the GeckoSessions when we call removeAndCloseAllSessions in response to the user tapping the notification for reasons that aren't entirely clear to me yet. Since we can't get the sessions, we "gracefully" fail by just removing them from the session manager without closing them, resulting in Gecko failing to clear data (and probably us leaking sessions as well, ouch). Adding contextIds just papers over the actual issue.

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Dylan Roeh (:droeh) (he/him) from comment #16)

(In reply to Dylan Roeh (:droeh) (he/him) from comment #15)

Setting a random contextId does indeed fix the issue, though I'm unconvinced it's the right approach. I'll dig into what our notification is doing a bit before putting this up for review.

Yeah, to followup here: the problem is that we are failing to get the GeckoSessions when we call removeAndCloseAllSessions in response to the user tapping the notification for reasons that aren't entirely clear to me yet. Since we can't get the sessions, we "gracefully" fail by just removing them from the session manager without closing them, resulting in Gecko failing to clear data (and probably us leaking sessions as well, ouch). Adding contextIds just papers over the actual issue.

Eek! Yeah we definitely want to fix this correctly then.

Comment 18

[Dylan Roeh (:droeh) (he/him)]

Jon, for context this is the bug underlying this PR I asked you to review: https://github.com/mozilla-mobile/focus-android/pull/4523

Comment 19

[Jonathan Almeida [:jonalmeida]]

We've merged this fix and I've verified it locally with the jsbin test site. It should go out in the 8.4.0 release.

Comment 20

[jesse.luoto]

It seems that 8.4.0 has been released. I just verified this fix with the public version from Play Store, so this issue can be marked as resolved. Thank you!