GV API to selectively delete user data using GeckoSession (Clear User Data API)

RESOLVED FIXED in Firefox 68

Status

enhancement
P1
normal
RESOLVED FIXED
9 months ago
5 days ago

People

(Reporter: jonalmeida, Assigned: esawin)

Tracking

(Depends on 1 bug)

unspecified
mozilla69
Unspecified
Android
Dependency tree / graph

Firefox Tracking Flags

(geckoview66 wontfix, firefox-esr60 wontfix, firefox66 wontfix, firefox67 wontfix, firefox68 fixed, firefox69 fixed)

Details

(Whiteboard: [geckoview:fenix:m5])

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

9 months ago
Looking at how Fennec clears data, I can see this is done via the event dispatcher[1]. Since this isn't the recommended way that we would like to do it in Android Components, a newer API would be nice.

For our current requirements, a catch-all method that deletes everything could work, but it's quite possible that we would want to eventually need something with more finer controls (e.g. delete only cookies).

Components specific discussions can be followed there as well[2].

[1]: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/preferences/PrivateDataPreference.java#61
[2]: https://github.com/mozilla-mobile/android-components/issues/644

Updated

6 months ago
Product: Firefox for Android → GeckoView
Reporter

Comment 1

5 months ago

Needed for r-b and fenix in the future.

Probably a way to select which parts of data to be cleared similar to fennec (with an new API):

GeckoBundle().apply {
    listOf(
        "openTabs",
        "history",
        "searchHistory",
        "downloadFiles",
        "formdata",
        "cookies_sessions",
        "cache",
        "offlineApps",
        "siteSettings",
        "syncedTabs",
        "passwords"
    ).map {
        putBoolean(it, true)
    }
}.also { bundle ->
    geckoSession.eventDispatcher.dispatch("Sanitize:ClearData", bundle)
}

Needed for r-b and fenix in the future.

Flagging Chris to add the right whiteboard labels and priorities.

Flags: needinfo?(cpeterson)

[geckoview:fenix:p2] nice-to-have for Fenix MVP unless someone speaks up.

See r-b #81 for the UI work.

Flags: needinfo?(cpeterson)
OS: Unspecified → Android
Priority: P3 → P2
Summary: API request for deleting use data using GeckoSession → API request for selectively deleting user data using GeckoSession
Whiteboard: [geckoview:fenix:p2]

Updated

3 months ago
Priority: P2 → P1
Summary: API request for selectively deleting user data using GeckoSession → GV API to selectively delete user data using GeckoSession
Whiteboard: [geckoview:fenix:p2] → [geckoview:fenix:m5]

Eugen, can you take a look at this bug for Fenix MVP?

The Fenix team would like a user option to clear all user data for Fenix MVP: mozilla-mobile/fenix#225. Selectively clearing data for individual sites or data types is not needed for Fenix MVP, but snorp says that we should probably go ahead and implement the GV API for selective clearing now.

Assignee: nobody → esawin
Flags: needinfo?(esawin)

(In reply to Chris Peterson [:cpeterson] from comment #5)

Eugen, can you take a look at this bug for Fenix MVP?

The Fenix team would like a user option to clear all user data for Fenix MVP: mozilla-mobile/fenix#225. Selectively clearing data for individual sites or data types is not needed for Fenix MVP, but snorp says that we should probably go ahead and implement the GV API for selective clearing now.

I think what I was getting at is that we need Sanitizer.jsm or something like it exposed in GV. But for the public API maybe we'd just start with a "nuke everything from orbit" API and add the selections later.

Summary: GV API to selectively delete user data using GeckoSession → GV API to selectively delete user data using GeckoSession (Clear User Data API)

James is concerned that GV's sanitizer stays in sync with desktop if new user data types are added.

Assignee

Comment 8

Last month

I think we could provide a powerful API here by exposing a subset of nsIClearDataService through our StorageController API.

Proposed API:

StorageController {
  ClearFlags {
    COOKIES
    NETWORK_CACHE
    IMAGE_CACHE
    HISTORY
    DOM_STORAGES
    AUTH_SESSIONS
    PERMISSIONS
    ALL_CACHES
    SITE_SETTINGS
    SITE_DATA
    ALL // 0xFFFFFF so shouldn't be affected by changing or new flags
  }

  void clearDataFromHost(String host, ClearFlags flags)

  void clearDataInTimeRange(Time from, Time to, ClearFlags flags)

  void clearData(ClearFlags flags)
}

The ClearFlags selection would be composed based on desktop's Sanitizer setup to provide a powerful yet not overloaded API.

The ClearFlags constants would need to maintain a consistent mapping to nsIClearDataService, which we hopefully can monitor through tests. Any additions to nsIClearDataService would need to be exposed through the GV API, if applicable.

Baku and snorp, do you see any issues with exposing this API to app consumers and would you agree with the proposed flag selection?

Flags: needinfo?(snorp)
Flags: needinfo?(esawin)
Flags: needinfo?(amarchesini)

Looks good to me!

Flags: needinfo?(snorp)

It seems a good idea. I have a couple of comments:

void clearDataFromHost(String host, ClearFlags flags)

This is fine, but:

  1. what about OriginAttributes? How to delete data for 1 particular container, for instance, for 1 particular GeckoView session ID?

  2. We need good documentation here, because this method would delete data for 1 single origin and I suspect it could be used wrongly: for instance, if we have 1 tab open, when deleting the first-party origin, firefox doesn't delete all the third party origins...
    This is actually an existing bug in 'forget about site'.

void clearDataInTimeRange(Time from, Time to, ClearFlags flags)

The time range is not fully supported by all the flags/storages. For some of them, we have to delete all. In your list, time range is not supported by: network cache, image cache, dom storages and auth sessions.

Flags: needinfo?(amarchesini)
Assignee

Comment 11

Last month

(In reply to Andrea Marchesini [:baku] from comment #10)

It seems a good idea. I have a couple of comments:

void clearDataFromHost(String host, ClearFlags flags)

This is fine, but:

  1. what about OriginAttributes? How to delete data for 1 particular container, for instance, for 1 particular GeckoView session ID?

Sorry for the confusion, I've left it out because that's already been reviewed in bug 1501108. I'm actually moving the clearSesssionContextData API out of bug 1501108 into this.

  1. We need good documentation here, because this method would delete data for 1 single origin and I suspect it could be used wrongly: for instance, if we have 1 tab open, when deleting the first-party origin, firefox doesn't delete all the third party origins...
    This is actually an existing bug in 'forget about site'.

Interesting, is there a way around that bug? Maybe you can link me to the existing bug so that I can make sure the docs reflect that properly.

void clearDataInTimeRange(Time from, Time to, ClearFlags flags)

The time range is not fully supported by all the flags/storages. For some of them, we have to delete all. In your list, time range is not supported by: network cache, image cache, dom storages and auth sessions.

I can add that to the docs, or we could leave this out of the API completely, since it isn't part of the current requirement.

Thanks!

Assignee

Comment 13

Last month

Depends on D31329

Assignee

Updated

27 days ago
Depends on: 1553454
Attachment #9065184 - Attachment is obsolete: true
Attachment #9065185 - Attachment is obsolete: true
Assignee

Comment 16

27 days ago

Removed the time range-based clearing because it has too many implementation restrictions that would need to be communicated and enforced.
Removed the session context-based clearing because it's still blocked on bug 1501108.

Moved the clear flags mapping into the JSM module to help with keeping the values in sync.

Extended and fixed tests. We can't load from the local test path due to the URI scheme restrictions and setting/getting localStorage or cookies via evaluateJS yielded a security error.
The tests now depend on Agi's WebExt support for evaluateJS to fix that.

Depends on: 1553515
Attachment #9066725 - Attachment description: Bug 1489669 - [1.3] Add Storage Controller API. → Bug 1489669 - [1.4] Add Storage Controller API.
Attachment #9066726 - Attachment description: Bug 1489669 - [2.1] Add test for Storage Controller API. → Bug 1489669 - [2.2] Add test for Storage Controller API.
Assignee

Comment 17

27 days ago

Landing the API patch now to unblock Fenix, will land test patch once bug 1553515 lands.

Comment 18

27 days ago
Pushed by esawin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1912d221317a
[1.4] Add Storage Controller API. r=baku,snorp

Comment 19

26 days ago
bugherder
Status: NEW → RESOLVED
Closed: 26 days ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Assignee

Comment 20

26 days ago

Comment on attachment 9066725 [details]
Bug 1489669 - [1.4] Add Storage Controller API.

Beta/Release Uplift Approval Request

  • User impact if declined: This a Fenix MVP release blocker.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This adds a new GeckoView API without affecting existing features.
    There are automated tests (second patch on this bug) and they pass, but they depend on bug 1553515 to land.
  • String changes made/needed: None
Attachment #9066725 - Flags: approval-mozilla-beta?

Comment on attachment 9066725 [details]
Bug 1489669 - [1.4] Add Storage Controller API.

new gv API, approved for 68.0b5

Attachment #9066725 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Regressions: 1559084
You need to log in before you can comment on or make changes to this bug.