784378 - Apps API - Clear Private Data

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Francis [:benfrancis]]

In gaia we need the ability to "clear private data" for web content inside the browser app and "clear app data" for an app from the settings app.

See:
https://github.com/mozilla-b2g/gaia/issues/1234
https://github.com/mozilla-b2g/gaia/issues/3124

for extensive discussions.

Comment 1

[Chris Lee [:clee]]

Marking blocking -- However, I would prioritize "clear private data" inside the browser app over "clear app data' per app.  

Both are important, but wanted to call this out if we had to prioritize.

Comment 2

[Andrew Overholt [:overholt]]

Justin offered to own this work.  I've marked Mounir's top-level "clear data jars" bug as a blocker of this bug.

Comment 3

[Mounir Lamouri (:mounir)]

Is this about clearing *all* data that the browser app is holding or only data that "tabs" own? I would tend to say it's the later. If that the case, it's not really related to bug 786293 which is about wiping data for an app. That bug would be more complex.

Comment 4

[Justin Lebar (not reading bugmail)]

> If that the case, it's not really related to bug 786293 which is about wiping data for an 
> app. That bug would be more complex.

I was thinking that clearing a browser tab's data is just like clearing an app's data.

Both a browser tab and an app have a storage context identified by (app-id, true/false).  When we clear an app, we clear the storage for (app-id, false).  When we clear a browser tab, we clear the storage for (app-id, true).

No?

Comment 5

[Ben Francis [:benfrancis]]

Mounir: To be clear, in Gaia's UX specs there are two requirements:

1) Inside the browser app settings, the ability to clear private data stored by all of the web content which has been accessed from inside the browser (but not the data stored by the browser app itself).
2) Inside the settings app, the ability to clear private data stored by a particular app, the apps are listed and there's a "Clear app data" button in the permissions panel for each app.

Comment 6

[Justin Lebar (not reading bugmail)]

Guessing LOE:M, although that's just for this bug -- the blockers (bug 786293) aren't counted in that estimate.

Comment 7

[Andrew Overholt [:overholt]]

Jonas said we can use the Settings app instead for now so removing blocker.

Comment 8

[Andrew Overholt [:overholt]]

Ben Francis helpfully clarified on IRC that this bug is essentially 2 things:  a) adding the platform support to allow the Settings app to be able to clear the bug and b) the actual API the title refers to.

I'm going to make this bug about adding the support and I've filed bug 791307 for the API itself.

Comment 9

[Ben Francis [:benfrancis]]

Sorry if I didn't explain myself clearly but that isn't what I meant.

The requirements here are:
1) Provide an API to clear the data stored by a particular app
2) Provide an API to clear the data stored by web content loaded inside an app

1 will be used by the settings app to clear app data for particular apps, 2 will be used by the browser app to clear private data stored by web content.

Justin provided an estimate of LOE:M for this bug, but that refers only to the work to hook the browser API up to existing functionality (for both requirements 1 and 2), which will be implemented elsewhere, specifically in all the bugs which are marked as blocking bug 786293

Does that make sense?

Comment 11

[Ben Francis [:benfrancis]]

I've put the bug back how it was to prevent further confusion

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Stealing.

Comment 13

[Ben Francis [:benfrancis]]

Thanks for taking this Ben!

This bug potentially covers clearing quite a few different things (cookies, IndexedDB, localstorage, browser cache, appcache...) in two different contexts (the data jar of an app, or the data jar which contains all the web content loaded inside an app).

Kev has clarified that the most essential requirement for the browser app is to be able to clear cookies for all the web content loaded inside the browser. The cache and "state" (not sure what that means) would come next.

Would you prefer to split the highest priority work into a separate bug with smaller scope and less dependencies, or are you happy to tackle this all at once, in time for the feature freeze?

The sooner you can give me an idea of what the API will look like, the sooner I can hook it up to the browser app.

Thanks!

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

To address requirement #2 from comment 9, the API will look like this:

  interface mozIDOMApplication {
    // ...
    void clearBrowserData([optional] in DOMString domain);
  }

So to use this you'd just do something like this:

  navigator.mozApps.getSelf().clearBrowserData();

I'm not sure if we care about requirement #1... I talked with sicking about this and we're not sure this can actually be done without just uninstalling the app. It's likely out of scope for this bug in any case.

Does this sound ok?

Comment 15

[Ben Francis [:benfrancis]]

I'm confused.

Requirement #2 from comment 9 is "Provide an API to clear the data stored by web content loaded inside an app". This is the API the browser app needs to clear private data stored for e.g. Google.com when the user loads that web page. If it is to be part of the Apps API (which sounds reasonable) I would expect this to look something like navigator.mozApps.getSelf().clearBrowserData(); but you say this is for requirement #1

Requirement #1 from comment 9 is "Provide an API to clear the data stored by a particular app". This is the API the settings app needs to clear data stored by a particular app (e.g. the browser stores bookmarks in IndexedDB and the settings app would be able to clear this data).

Both of these requirements are currently within the scope of this bug and are blocking https://github.com/mozilla-b2g/gaia/issues/3124 and https://github.com/mozilla-b2g/gaia/issues/1234 respectively.

Could you clarify which of these you're working on, and move the parts you're not working on out to separate bug so it can be tracked separately if that's what you'd prefer?

Thanks!

Comment 16

[Justin Lebar (not reading bugmail)]

> Could you clarify which of these you're working on, and move the parts you're not working on out to 
> separate bug so it can be tracked separately if that's what you'd prefer?

We have so many different bugs on this, I cannot keep them straight.  At this point, I personally would prefer to morph this bug into whatever it is that bent is doing.  We do not gain much by continuing to move this work around into different bugs.

> but you say this is for requirement #1

That's not how I read comment 14.  I read comment 14 as saying "this is my API for requirement #2, and I'm not sure if we care about requirement #1; it may not be possible, and is likely out of scope."

Comment 17

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Ben Francis [:benfrancis] from comment #15)
> but you say this is for requirement #1

Nope, I said that that API will address requirement #2.

> Requirement #1 from comment 9 is "Provide an API to clear the data stored by
> a particular app". This is the API the settings app needs to clear data
> stored by a particular app (e.g. the browser stores bookmarks in IndexedDB
> and the settings app would be able to clear this data).

Right. I'm not convinced that this can be done generically without just uninstalling the app, but even setting aside my doubts, this should be a different bug.

Let's let this bug track the platform side of requirement #2 (https://github.com/mozilla-b2g/gaia/issues/3124) and get something else going for requirement #1 (https://github.com/mozilla-b2g/gaia/issues/1234).

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to ben turner [:bent] from comment #17)
> get something else going for requirement #1

Bug 792892

Comment 19

[Ben Francis [:benfrancis]]

Justin you're right, I got confused, sorry.

Great, so this bug is about Clear Private Data which blocks https://github.com/mozilla-b2g/gaia/issues/3124

and Bug 792892 is about Clear App Data (I will rename), which blocks https://github.com/mozilla-b2g/gaia/issues/1234

Thank you :)

Now, if this is going to be part of the Apps API rather than the Browser API (which sounds perfectly reasonable), then does this mean the browser app will need an additional permission or will it always work via getSelf() without permissions?

Also if it is part of the apps API, we should rename the titles.

Comment 20

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Actually, the interface is not going to mess with domains at the moment:

  interface mozIDOMApplication {
    // ...
    void clearBrowserData();
  }

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

The internal notification is going to use the observer service.

  topic: "mozapps-clear-data"
  subject: mozIApplicationClearPrivateDataParams
  data: null

  interface mozIApplicationClearPrivateDataParams {
    readonly attribute unsigned long appId;
    readonly attribute boolean browserOnly;
  };

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Here's what consumers of this notification should do:

If the browserOnly flag is false, clear all data associated with the specified appID. 

If the browserOnly flag is true, *only* clear data which has the isInBrowserElement (aka browser-flag) set to true.


The appID will never be NO_APP or UNKNOWN_APP.

Comment 23

[Justin Lebar (not reading bugmail)]

Notice that this will get you bug 792892 for free.  :)

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

That's not entirely by accident :) This is what we're planning on using for both various "clear private data" as well as for uninstall.

Comment 25

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to ben turner [:bent] from comment #21)
> The internal notification is going to use the observer service.
>   topic: "mozapps-clear-data"

Hm, everything else uses "webapps-*" instead, so it's actually going to be "webapps-clear-data".

Comment 26

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

Not sure who should review this... jlebar, shall we start with you?

Comment 27

[Justin Lebar (not reading bugmail)]

Okay, but I probably can't get to this until tomorrow.

Comment 28

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v2

It's a pain to make instances of some observer class just to catch this notification. Mounir apparently ran into this already.

This is basically the same patch except that I'm using the category manager as well as the observer service to notify components.

Comment 29

[Justin Lebar (not reading bugmail)]

> This is basically the same patch except that I'm using the category manager as well as the observer 
> service to notify components.

Can you point me to some code which uses the category manager, so I can see how much better this is?  (In general, using two mechanisms for notifying instead of one feels like overkill, particularly when just notifying the category service takes so much boilerplate.)

Comment 30

[Justin Lebar (not reading bugmail)]

Comment on attachment 664183 [details] [diff] [review]
Patch, v2

>diff --git a/dom/interfaces/apps/nsIDOMApplicationRegistry.idl b/dom/interfaces/apps/nsIDOMApplicationRegistry.idl
>--- a/dom/interfaces/apps/nsIDOMApplicationRegistry.idl
>+++ b/dom/interfaces/apps/nsIDOMApplicationRegistry.idl
>@@ -36,16 +36,19 @@ interface mozIDOMApplication  : nsISuppo
>   /*
>    * fires a nsIDOMApplicationEvent when a change in appcache download or status happens
>    */
>   attribute nsIDOMEventListener onprogress;
> 
>   /* startPoint will be used when several launch_path exists for an app */
>   nsIDOMDOMRequest launch([optional] in DOMString startPoint);
>   nsIDOMDOMRequest uninstall();
>+
>+  /* Clear data that has been collected through mozbrowser elements. */
>+  void clearBrowserData();
> };

Change the UUID.  :)

>diff --git a/dom/apps/src/Webapps.js b/dom/apps/src/Webapps.js

>+  clearBrowserData: function() {
>+    let browserChild =
>+      BrowserElementPromptService.getBrowserElementChildForWindow(this._window);
>+    browserChild.messageManager.sendAsyncMessage("Webapps:ClearBrowserData");
>+  },

Please add a null-check here; not all windows have a BEC.  (Particularly
outside of b2g, and we share this code with desktop apps, right?)

I'd prefer that we moved getBrowserElementChildForWindow out of the "prompt
service" and into something else (BrowserElementLookupService?), but if you
don't want to do that here, that's fine.  Just please file a follow-up that I
can take?

>diff --git a/dom/apps/src/Webapps.jsm b/dom/apps/src/Webapps.jsm
>+  _notifyCategoryAndObservers: function(subject, topic, data) {

This is pretty complicated in comparison to observerService.notifyObservers().
I'm willing to be convinced by a demonstration of how much easier this makes
life for observers, but another question is, why should this particular
notification get special treatment?  Doing things differently in one place
(i.e., special treatment) just makes our code confusing.

>diff --git a/dom/browser-element/BrowserElementChild.js b/dom/browser-element/BrowserElementChild.js
>--- a/dom/browser-element/BrowserElementChild.js
>+++ b/dom/browser-element/BrowserElementChild.js
>@@ -670,16 +670,21 @@ BrowserElementChild.prototype = {
> 
>       sendAsyncMsg('securitychange', {state: stateDesc, extendedValidation: isEV});
>     },
> 
>     onStatusChange: function(webProgress, request, status, message) {},
>     onProgressChange: function(webProgress, request, curSelfProgress,
>                                maxSelfProgress, curTotalProgress, maxTotalProgress) {},
>   },
>+
>+  // Expose the message manager for WebApps and others.
>+  get messageManager() {
>+    return global;

Can we just return {sendAsyncMessage: global.sendAsyncMessage, sendSyncMessage:
global.sendSyncMessage, addMessageListener: global.addMessageListener} instead
of the whole global?

>diff --git a/dom/browser-element/BrowserElementParent.js b/dom/browser-element/BrowserElementParent.js
>--- a/dom/browser-element/BrowserElementParent.js
>+++ b/dom/browser-element/BrowserElementParent.js
>@@ -8,16 +8,21 @@ let Cu = Components.utils;
> let Ci = Components.interfaces;
> let Cc = Components.classes;
> let Cr = Components.results;
> 
> Cu.import("resource://gre/modules/Services.jsm");
> Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> Cu.import("resource://gre/modules/BrowserElementPromptService.jsm");
> 
>+XPCOMUtils.defineLazyGetter(this, "DOMApplicationRegistry", function () {
>+  Cu.import("resource://gre/modules/Webapps.jsm");
>+  return DOMApplicationRegistry;
>+});
>+
> const NS_PREFBRANCH_PREFCHANGE_TOPIC_ID = "nsPref:changed";
> const BROWSER_FRAMES_ENABLED_PREF = "dom.mozBrowserFramesEnabled";
> const TOUCH_EVENTS_ENABLED_PREF = "dom.w3c_touch_events.enabled";
> 
> function debug(msg) {
>   //dump("BrowserElementParent - " + msg + "\n");
> }
> 
>@@ -245,16 +250,27 @@ function BrowserElementParent(frameLoade
>   // down to the child.
>   this._window.addEventListener('mozvisibilitychange',
>                                 this._ownerVisibilityChange.bind(this),
>                                 /* useCapture = */ false,
>                                 /* wantsUntrusted = */ false);
> 
>   // Insert ourself into the prompt service.
>   BrowserElementPromptService.mapFrameToBrowserElementParent(this._frameElement, this);
>+
>+  // If this browser represents an app then let the Webapps module register for
>+  // any messages that it needs.
>+  let appManifestURL = this._frameElement.getAttribute('mozapp');

Please do nsIMozBrowserFrame.appManifestURL; that will (soon) do the
appropriate permission check and return the empty string if this frame doesn't
have permission to be an app.

I'd like to see the updated code if you decide to move the code out of BrowserElementPromptService.  If you decide not to do that, feel free to reset r? without updating the patch once you show me how the category service listener is registered.

Comment 31

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Bent: This isn't yet sending the notification during app uninstall, is it? Would you mind adding that in this bug so that people don't have to hook up listening to both webapp uninstall and clear-private-data for now.

Comment 32

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Justin Lebar [:jlebar] from comment #29)
> Can you point me to some code which uses the category manager, so I can see
> how much better this is?  (In general, using two mechanisms for notifying
> instead of one feels like overkill, particularly when just notifying the
> category service takes so much boilerplate.)

I think we should get rid of the observerservice notification and *just* use the category manager.

Comment 33

[Justin Lebar (not reading bugmail)]

We use the observer service /everywhere/ in Gecko.  Are you suggesting we should deprecate it, or that this situation is somehow different from the other situations where we use the observer service?

I'd really like to see some listener code so I can better understand what you're trying to accomplish here.

Comment 34

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 664183 [details] [diff] [review]
Patch, v2

Ok, chatted on irc, I think we're on the same page here now!

Comment 35

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Filed bug 794255 to track the refactoring.

Comment 36

[Justin Lebar (not reading bugmail)]

Comment on attachment 664183 [details] [diff] [review]
Patch, v2

Can we make a deal to rip out the category manager code if nobody is using it in a month?

Comment 37

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Justin Lebar [:jlebar] from comment #36)
> Can we make a deal to rip out the category manager code if nobody is using
> it in a month?

Sure! (But I'm planning on using it for bug 786295 this week ;) )

Comment 38

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Review changes

Here are the changes I've made based on your comments.

Comment 39

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Jonas Sicking (:sicking) from comment #31)
> Bent: This isn't yet sending the notification during app uninstall, is it?
> Would you mind adding that in this bug so that people don't have to hook up
> listening to both webapp uninstall and clear-private-data for now.

Bug 784378.

Comment 40

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to ben turner [:bent] from comment #39)
> Bug 784378.

Er, bug 794563.

Comment 41

[Justin Lebar (not reading bugmail)]

Comment on attachment 665051 [details] [diff] [review]
Review changes

These changes look great.

Comment 42

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Final patch

Comment 43

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5d656109b8a

Comment 44

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Grr, forgot to 'hg add'...

https://hg.mozilla.org/integration/mozilla-inbound/rev/7771d1d6f0d8

Comment 45

[Jason Duell]

Comment on attachment 665078 [details] [diff] [review]
Final patch

Review of attachment 665078 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/interfaces/apps/mozIApplicationClearPrivateDataParams.idl
@@ +7,5 @@
> +
> +#include "nsISupports.idl"
> +
> +[scriptable, uuid(ba0e6c8e-8c03-4b9b-8f9b-4fb14216f56e)]
> +interface mozIApplicationClearPrivateDataParams

This needs to inherit from nsISupports.

Comment 46

[Ben Francis [:benfrancis]]

A couple of questions about using this API:

1) It looks like there's no callback of any kind to notify content when the clearing operation has completed, is this correct? The reason that I ask is that the UX spec requires the button to be disabled once the operation is complete to indicate there is now no data to clear (https://www.dropbox.com/sh/ygwfxk6chpshxdj/w_SAwiqXhu/Apps/Browser/R1_Browser-Settings_v4.pdf). I can fake this by just immediately disabling the button until this screen is next displayed, but I just want to make sure.
2) What types of data will be cleared by this API in v1? The prompt in the UI mockups say "Saved passwords and cookies will be deleted." which I'm guessing isn't entirely accurate?

Thanks!

Comment 47

[Mounir Lamouri (:mounir)]

(In reply to Ben Francis [:benfrancis] from comment #46)
> 1) It looks like there's no callback of any kind to notify content when the
> clearing operation has completed, is this correct? The reason that I ask is
> that the UX spec requires the button to be disabled once the operation is
> complete to indicate there is now no data to clear
> (https://www.dropbox.com/sh/ygwfxk6chpshxdj/w_SAwiqXhu/Apps/Browser/
> R1_Browser-Settings_v4.pdf). I can fake this by just immediately disabling
> the button until this screen is next displayed, but I just want to make sure.

You should do that, indeed.

> 2) What types of data will be cleared by this API in v1? The prompt in the
> UI mockups say "Saved passwords and cookies will be deleted." which I'm
> guessing isn't entirely accurate?

What *might* be deleted is: localstorage, app cache, cookies and IndexedDB.
Saved passwords will not be deleted. Actually, is B2G saving passwords?

Comment 48

[Ben Francis [:benfrancis]]

(In reply to Mounir Lamouri (:mounir) from comment #47)
> What *might* be deleted is: localstorage, app cache, cookies and IndexedDB.

OK, thanks.

> Saved passwords will not be deleted. Actually, is B2G saving passwords?

Not as far as I know.

Comment 49

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b5d656109b8a
https://hg.mozilla.org/mozilla-central/rev/7771d1d6f0d8

Comment 50

[Ben Francis [:benfrancis]]

w00t! Thanks guys!

Comment 51

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Jason Duell (:jduell) from comment #45)
> This needs to inherit from nsISupports.

Bug 794973.

Comment 52

[Chris Lee [:clee]]

Awesome work, thanks everyone.