Last Comment Bug 508167 - NPAPI additions for clearing recent history (e.g. for "flash cookies")
: NPAPI additions for clearing recent history (e.g. for "flash cookies")
Status: RESOLVED FIXED
[softblocker]
: dev-doc-needed
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: All All
: -- enhancement with 12 votes (vote)
: ---
Assigned To: dwitte@gmail.com
:
:
Mentors:
: 618461 (view as bug list)
Depends on: 623695 633463
Blocks: 632789 290456 508068 565561 625495 625496 625497
  Show dependency treegraph
 
Reported: 2009-08-03 18:34 PDT by Jesse Ruderman
Modified: 2011-09-18 18:53 PDT (History)
42 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
npapi header changes (7.83 KB, patch)
2011-01-05 19:20 PST, dwitte@gmail.com
jaas: review+
Details | Diff | Splinter Review
header changes v2 (8.28 KB, patch)
2011-01-05 23:25 PST, dwitte@gmail.com
dwitte: review+
stuart.morgan+bugzilla: review+
Details | Diff | Splinter Review
implementation v1 (46.26 KB, patch)
2011-01-12 21:42 PST, dwitte@gmail.com
no flags Details | Diff | Splinter Review
implementation (6.64 KB, patch)
2011-02-01 21:00 PST, dwitte@gmail.com
no flags Details | Diff | Splinter Review
implementation (51.23 KB, patch)
2011-02-01 21:01 PST, dwitte@gmail.com
no flags Details | Diff | Splinter Review
implementation v2 (52.75 KB, patch)
2011-02-02 17:10 PST, dwitte@gmail.com
jaas: review-
Details | Diff | Splinter Review
implementation v3 (53.63 KB, patch)
2011-02-03 16:17 PST, dwitte@gmail.com
jaas: review+
Details | Diff | Splinter Review
v4 (53.81 KB, patch)
2011-02-08 14:09 PST, dwitte@gmail.com
dwitte: review+
mbeltzner: approval2.0+
Details | Diff | Splinter Review

Description Jesse Ruderman 2009-08-03 18:34:14 PDT
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 2009-08-03 18:36:31 PDT
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] 2009-08-11 16:40:39 PDT
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] 2009-08-19 14:17:55 PDT
Please do propose an API. :)
Comment 4 spora 2010-03-26 23:54:42 PDT
Please do propose an API. :)
Comment 5 Sid Stamm [:geekboy or :sstamm] 2010-05-27 10:41:38 PDT
Proposed API:

https://wiki.mozilla.org/Plugins:ClearPrivacyData#Current_Proposal
Comment 6 Trevor 2010-06-29 20:29:36 PDT
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 dwitte@gmail.com 2011-01-05 16:20:36 PST
-> me. We have a final-enough proposal to implement this now.
Comment 8 dwitte@gmail.com 2011-01-05 19:20:42 PST
Created attachment 501561 [details] [diff] [review]
npapi header changes
Comment 9 Josh Aas 2011-01-05 19:40:59 PST
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 dwitte@gmail.com 2011-01-05 23:25:32 PST
Created attachment 501604 [details] [diff] [review]
header changes v2

Fixed, and builds on all plats. Over to you!
Comment 11 Stuart Morgan 2011-01-06 11:44:56 PST
Comment on attachment 501604 [details] [diff] [review]
header changes v2

LGTM for upstream landing.
Comment 12 Josh Aas 2011-01-06 12:37:03 PST
Pushed to npapi-headers as r32. Filed bug 623695 about syncing to mozilla-central.
Comment 13 mlissner@michaeljaylissner.com 2011-01-06 13:00:56 PST
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 dwitte@gmail.com 2011-01-11 22:26:03 PST
(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 dwitte@gmail.com 2011-01-12 21:42:58 PST
Created attachment 503408 [details] [diff] [review]
implementation v1

Fairly simple stuff. Includes tests!
Comment 16 Emmy 2011-01-12 22:08:34 PST
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] 2011-01-13 04:39:09 PST
Will this make Firefox 4?
Comment 18 Mike Beltzner [:beltzner, not reading bugmail] 2011-01-13 12:04:17 PST
Can't commit to this for Firefox 4 at this time, no, but that doesn't mean we should stop working on it.
Comment 19 dwitte@gmail.com 2011-01-13 12:23:37 PST
*** Bug 618461 has been marked as a duplicate of this bug. ***
Comment 20 Mike Beltzner [:beltzner, not reading bugmail] 2011-01-13 14:39:15 PST
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 dwitte@gmail.com 2011-01-24 16:13:31 PST
Comment on attachment 503408 [details] [diff] [review]
implementation v1

Obsolete -- this isn't quite what we want. New patch written, pending tests!
Comment 22 dwitte@gmail.com 2011-02-01 21:00:28 PST
Created attachment 509010 [details] [diff] [review]
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 dwitte@gmail.com 2011-02-01 21:01:00 PST
Created attachment 509011 [details] [diff] [review]
implementation

Sigh, wrong patch.
Comment 24 dwitte@gmail.com 2011-02-02 17:10:06 PST
Created attachment 509305 [details] [diff] [review]
implementation v2

I forgot to add tests for IDN domains (and found a bug when doing so!). Tweaked!
Comment 25 Josh Aas 2011-02-03 11:39:57 PST
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 dwitte@gmail.com 2011-02-03 16:17:15 PST
Created attachment 509628 [details] [diff] [review]
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 2011-02-05 13:32:06 PST
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 dwitte@gmail.com 2011-02-07 11:44:03 PST
(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 2011-02-07 13:54:07 PST
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 dwitte@gmail.com 2011-02-07 14:01:14 PST
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 2011-02-07 15:14:01 PST
(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 dwitte@gmail.com 2011-02-08 14:09:22 PST
Created attachment 510779 [details] [diff] [review]
v4

Thanks. Other nits addressed. This is ready for approval!
Comment 33 Mike Beltzner [:beltzner, not reading bugmail] 2011-02-08 14:11:08 PST
Comment on attachment 510779 [details] [diff] [review]
v4

a=beltzner, yay!
Comment 34 dwitte@gmail.com 2011-02-08 14:18:43 PST
Fixed!

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

Note You need to log in before you can comment on or make changes to this bug.