508167 - NPAPI additions for clearing recent history (e.g. for "flash cookies")

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, enhancement)

Description

[Jesse Ruderman]

The Flash guys want to play nicely with Firefox privacy settings and features, but NPAPI doesn't give them a good way to do this (see bug 508068 comment 4 and bug 290456 comment 22).

I guess we'd want the API to be able to support private browsing and clearing cookies, at least.  Or perhaps Flash could delegate the storage itself to Firefox, so all cookie-related features in Firefox and extensions can be used to view and control Flash cookies as well.

Comment 1

[Ian Melven]

there already is an NPAPI for supporting private browsing which we are taking advantage of in a future release  - the one we would like is the one to hook into Clear Private Data to delete our LSO's. 

we can propose one on plugin-futures to get the ball rolling if that makes sense.

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

A recent Berkeley paper discusses the use of LSOs by top 100 sites, including using them to cause HTTP cookies to be repopulated after being cleared:

http://papers.ssrn.com/sol3/papers.cfm?abstract_id=1446862

The authors call for a way to connect the clear recent history feature to plugins like Flash.

Comment 3

[Lucas Adamski [:ladamski]]

Please do propose an API. :)

Comment 4

[fraster]

Please do propose an API. :)

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

Proposed API:

https://wiki.mozilla.org/Plugins:ClearPrivacyData#Current_Proposal

Comment 6

[Trevor]

Adobe could solve a lot of this "problem" with some changes to how Flash Player works:
1. Change default settings from "Always Allow" to "Always Ask" (If domains can save cookies)
2. Acknowledge private browsing mode [This is already sorted?]
3. Add a per object setting, not just a per domain. (You may want to save your high score, but not allow the site tracking cookie)
4. Create an easy to use management interface that all browsers can integrate.
And finally before anyone gets too excited about developing this NPAPI,
5. Keep track of when cookies were created, modified etc so this proposed NPAPI will actually be useful.

Comment 7

[[e-mail]]

-> me. We have a final-enough proposal to implement this now.

Comment 8

[[e-mail]]

Attachment: npapi header changes

Comment 9

[Josh Aas]

Comment on attachment 501561 [details] [diff] [review]
npapi header changes

>+/*
>+ * Flags for NPP_ClearSiteData.
>+ */
>+#define NP_CLEAR_ALL   0
>+#define NP_CLEAR_CACHE 1 << 0

Move these flags so that they are right underneath "#define NP_MAXREADY (((unsigned)(~0)<<1)>>1)". They belong at the end of that section, not in the "Error and Reason Code definitions".

Otherwise if this compiles on all the major platforms then I think it's good. We'll need to commit this to the standard headers and then sync to the Mozilla tree to make sure there is no difference between the headers. I'll take care of committing to the standard headers some time this week.

Comment 10

[[e-mail]]

Attachment: header changes v2

Fixed, and builds on all plats. Over to you!

Comment 11

[Stuart Morgan]

Comment on attachment 501604 [details] [diff] [review]
header changes v2

LGTM for upstream landing.

Comment 12

[Josh Aas]

Pushed to npapi-headers as r32. Filed bug 623695 about syncing to mozilla-central.

Comment 13

[[e-mail]]

If I understand this bug correctly, once this is fixed, FF will provide a mechanism for plugins to delete their cookies, LSOs, etc, using FF's tools.

Is there a related bug on the Adobe side where we can monitor Adobe's implementation of this feature into its products?

Comment 14

[[e-mail]]

(In reply to comment #13)
> Is there a related bug on the Adobe side where we can monitor Adobe's
> implementation of this feature into its products?

That's a good question. I don't believe so, but Josh would know for sure.

Comment 15

[[e-mail]]

Attachment: implementation v1

Fairly simple stuff. Includes tests!

Comment 16

[Emmy]

Hi,

Adobe is ready to coordinate with Mozilla on timing of a release to support this feature. I created an Adobe bug so people can track progress: https://bugs.adobe.com/jira/browse/FP-6044

best,
Emmy

Comment 17

[Paul [pwd]]

Will this make Firefox 4?

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Can't commit to this for Firefox 4 at this time, no, but that doesn't mean we should stop working on it.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Just added bug 625495, bug 625496 and bug 625497 for hooking this in to our existing UI touchpoints for deleting cookies. Based on existing user surprise and feedback, I believe we can expect them to expect "cookies" to cover web cookies and flash LSOs.

Comment 21

[[e-mail]]

Comment on attachment 503408 [details] [diff] [review]
implementation v1

Obsolete -- this isn't quite what we want. New patch written, pending tests!

Comment 22

[[e-mail]]

Attachment: implementation

This reflects the NPAPI bits into an API suitable for the browser, detailed in nsIPluginHost.idl: clearSiteData and siteHasData. Right now, siteHasData is only used in tests. We'll need it for future work (Site Privacy pane), and the code is basically common to clearSiteData, so I've just added it now.

I don't really know if we want to be potentially creating plugins (see EnsurePlugin) via clearSiteData, but without it, I was certainly seeing cases where we'd have a plugintag with an uninitialized library. And I imagine we want to clear data for plugins that haven't been initialized yet, rather than just skipping them.

Otherwise, this is all fairly simple stuff. There are a bunch of tests here; there will be more in bug 625496 and bug 625497 for those respective client bits.

Comment 23

[[e-mail]]

Attachment: implementation

Sigh, wrong patch.

Comment 24

[[e-mail]]

Attachment: implementation v2

I forgot to add tests for IDN domains (and found a bug when doing so!). Tweaked!

Comment 25

[Josh Aas]

Comment on attachment 509305 [details] [diff] [review]
implementation v2

>+bool
>+PluginModuleChild::AnswerClearSiteDataSupported(bool *aBoolVal)
>+{
>+    *aBoolVal = !!mFunctions.clearsitedata && !!mFunctions.getsiteswithdata;
>+    return true;
>+}

Strictly speaking, a plugin could support only clearing *all* site data by just implementing the clearing function without the discovery function. I'm not sure this is a case we need to support, but it would be nice to support it if it's not too much trouble.

>     // Provide 'NPP_URLRedirectNotify' if it is supported by the plugin.
>     bool urlRedirectSupported = false;
>-    CallURLRedirectNotifySupported(&urlRedirectSupported);
>+    unused << CallURLRedirectNotifySupported(&urlRedirectSupported);
>     if (urlRedirectSupported) {
>       aFuncs->urlredirectnotify = NPP_URLRedirectNotify;
>     }
>+
>+    // Likewise for 'NPP_ClearSiteData' and 'NPP_GetSitesWithData'.
>+    unused << CallClearSiteDataSupported(&mClearSiteDataSupported);

I'd really prefer that the function table provided by the PluginModuleParent reflect the plugin's table exactly in terms of things being NULL or not. See the NPP_URLRedirectNotify case directly above your new code here. If you don't do that then we can't write normal code in our core NPAPI impl. Later in the patch, in nsPluginHost::ClearSiteData, you should be calling NPAPI functions provided by the plugin the way we do in "nsNPAPIPluginInstance::SetWindow", for example.

>+nsresult
>+PluginModuleParent::NPP_ClearSiteData(const char* site, uint64_t flags,
>+                                      uint64_t maxAge)

The signature of the NPAPI function exposed by the PluginModuleParent should match the actual NPAPI signature so it makes no difference whether we call on an actual plugin library or a PluginModuleParent. The return value here should be an NPError.

>+nsresult
>+PluginModuleParent::NPP_GetSitesWithData(InfallibleTArray<nsCString>& result)

Same here.

>+    bool mClearSiteDataSupported;

You won't need to store this once support status is indicated by a nullable entry in the function table.

Just these changes will require a good amount of restructuring so lets take care of that before finishing the review. The API logic itself looks good so far.

Comment 26

[[e-mail]]

Attachment: implementation v3

Good point about the granularity of ClearSiteData vs GetSitesWithData support. I split those out.

The function pointer stuff actually isn't possible (easily): all those NPP_* fnptrs are declared static on PluginModuleParent, and we use the instance pointer to retrieve the PluginModuleParent instance to make the call. There's no instance argument for these functions, so we have no way to get the PluginModuleParent instance. The only way to do it would be to allocate a closure specially for that instance, and burn 'this' into it. Which is possible with something like libffi, but I don't think we want to go there. :)

Also, I asked cjones about the rpc semantics: we actually require rpc on parent->child messages. Sync calls can deadlock if the child is sending the parent a message concurrently.

Comment 27

[Josh Aas]

Comment on attachment 509628 [details] [diff] [review]
implementation v3

>+  rpc OptionalFunctionsSupported()
>+    returns (bool aURLRedirectNotify, bool aClearSiteData,
>+             bool aGetSitesWithData);

I don't like this, it's not friendly to expansion of optional functions. Just make this three separate functions. That will also make reading the code easier.

>+nsresult
>+PluginPRLibrary::NPP_GetSitesWithData(InfallibleTArray<nsCString>& result)
>+{
>+  if (!mNPP_GetSitesWithData)
>+    return NS_ERROR_NOT_AVAILABLE;
>+
>+  char** sites = mNPP_GetSitesWithData();
>+  if (!sites)
>+    return NS_ERROR_FAILURE;
>+
>+  char** iterator = sites;
>+  while (*iterator) {
>+    result.AppendElement(*iterator);
>+    NS_Free(*iterator);
>+    ++iterator;
>+  }
>+  NS_Free(sites);
>+
>+  return NS_OK;
>+}

I think returning a NULL pointer to the array is an acceptable alternative to an array with a single NULL entry per the spec (by default). Here they are treated differently - a NULL pointer to the array will result in NS_ERROR_FAILURE being returned, an array with a single NULL entry will return NS_OK. In the OOPP case, they are treated as the same thing (returning NS_OK for both). This is inconsistent. Above this level, NULL list vs. a single-NULL-entry list seems irrelevant, we should probably treat them as the same thing here.

>+nsPluginHost::EnumerateSiteData(const nsACString& domain,
>+                                const nsTArray<nsCString>& sites,
>+                                InfallibleTArray<nsCString>& result,
>+                                bool earlyReturn)

Change "earlyReturn" to something like "firstMatchOnly".

>+  if (isIP || rv == NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS) {
>+    // The base domain is the site itself. However, we must be careful to
>+    // normalize.
>+    baseDomain = domain;
>+    rv = NormalizeHostname(baseDomain);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+  } else if (NS_FAILED(rv)) {

Get rid of the extra newline after NS_ENSURE_SUCCESS.

>+        rv = NormalizeHostname(siteBaseDomain);
>+        NS_ENSURE_SUCCESS(rv, rv);
>+
>+      } else if (NS_FAILED(rv)) {
>+        return rv;
>+      }

Another extra newline in there.

>+  if (!tag)
>+    return NS_ERROR_NOT_AVAILABLE;

Generally I prefer brackets around all "if" statements in the module. Don't spend a lot of time on this or bother with existing non-bracketed cases but where you can do it easily, please do.

>+    if ((site == NULL || data.site.compare(site) == 0) &&

"site == NULL" -> "!site"

>+  // Allocate the maximum possible size the list could be.
>+  result = static_cast<char**>(NPN_MemAlloc((length + 1) * sizeof(char*)));
>+  result[length] = NULL;
>+
>+  if (length == 0)
>+    return result;

Add a comment that you're intending to represent no site data as an array with a single NULL entry rather than a NULL pointer to an array. I believe either representation is valid per the spec.

>+  // Add strings to the result array, and nullterminate.

"nullterminate" -> "null terminate"

Looks great, thanks Dan!

Comment 28

[[e-mail]]

(In reply to comment #27)
> >+  rpc OptionalFunctionsSupported()
> >+    returns (bool aURLRedirectNotify, bool aClearSiteData,
> >+             bool aGetSitesWithData);
> 
> I don't like this, it's not friendly to expansion of optional functions. Just
> make this three separate functions. That will also make reading the code
> easier.

The reason I did this is that each sync call is expensive -- it blocks the main process (GUI thread) waiting for a response. For Fennec in particular, this can be very expensive. For simple cases like this it's definitely worth it to consolidate.

Why do you say it's not friendly to expansion? IPDL APIs are easy to modify since they're not intended for use by anything other than internal code, so adding extra parameters is trivial.

Comment 29

[Josh Aas]

I don't think that function is called very often, I don't think sync calls are *that* expensive (and the function itself does next to nothing), and the cost is a very minor component of the higher-level operation within which the calls happen. In the case where you only care about one value the cost for the better API is the same. I don't think the savings are worth what would otherwise be poor API design.

That said, not worth delaying over this. If you disagree I'm happy to do it your way.

Comment 30

[[e-mail]]

What the function does isn't really relevant, the cost is just waiting on IPC. In Fennec testing, we found that the typical delay is on the order of milliseconds. Remember that the kernel has to context-switch, other processes/threads might get their turn for a timeslice, the child process has to do its work, and then we have to do all that again to get back to the parent.

It's up to you, but for something like this I'd always lean toward consolidating calls.

Comment 31

[Josh Aas]

(In reply to comment #30)

> It's up to you, but for something like this I'd always lean toward
> consolidating calls.

I'm fine with that.

Comment 32

[[e-mail]]

Attachment: v4

Thanks. Other nits addressed. This is ready for approval!

Comment 33

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 510779 [details] [diff] [review]
v4

a=beltzner, yay!

Comment 34

[[e-mail]]

Fixed!

http://hg.mozilla.org/mozilla-central/rev/d0ea866fef6e