468877 - Expose the private browsing mode's current status and transitions through NPAPI

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[(no longer active)]

As suggested by dveditz at the private browsing security review, some plugin vendors such as Adobe are willing to support private browsing mode if it's exposed through the NPAPI.

This needs a spec, and once we have that, the implementation should be fairly trivial.

We would probably need an API to query the current status of the private browsing mode, and an API for getting notified of changes to the private browsing mode at least.

Comment 1

[u88484]

Ehsan, I'm just curious on what use Adobe would have in support pb mode?  Do you have any documentation on use cases for this that I could read?

Comment 2

[Daniel Veditz [:dveditz]]

For instance, Flash could not return previously saved local data and not store additional data. Other plugins may have similar types of local storage, or a playlist-history kind of mechanism.

Comment 3

[Peter Lairo]

I suspect a non-trivial percentage of PB cases will be for porn browsing (e.g., college dorm with nosy room-mates). Porn sites use Flash (so I heard ;-) ). If Firefox doesn't protect against these cases, then I fear the publicity on release of Firefox 3.1 for PB will be substantially dampened.

--> wanted1.9.1 ?

PS. I hope Adobe can update their Flash Player in time too.

Comment 4

[James May [:fowl2]]

Would if be possible for plugins to identify whether they supported PB mode, or could that be inferred from registering for a PB notification.

Comment 5

[(no longer active)]

Before jumping to an implementation, this would need a spec, so that other browser vendors which implement NPAPI can support it, and also plugin vendors would have to deal with a single interface for supporting the private browsing mode, no matter which browser hosts their plugin.

Comment 6

[Josh Aas]

Here is the spec I am proposing. This is evolved from ideas that came from several people/groups.

By default plugins should assume that private mode is off. When the browser turns private mode on it will call NPP_SetValue for "NPPVprivateModeBool" on the plugin with a non-null value for the argument pointer. Plugins should check the boolean value of the pointer itself, not what it points to. When the browser turns private mode off it will call NPP_SetValue on the plugin with a null value for the argument pointer.

Browsers should inform plugins of private mode state after calling NPP_Initialize in addition to informing upon private mode state changes.

Changes in privacy mode affect new streams only - not previously created streams.

Comment 7

[(no longer active)]

So this way if a plugin manages an internal state flag, it shouldn't need querying for the current status, right?

Also, I'm not sure how this stuff works exactly, but is it safe to assume that once we implement this, other browser vendors would implement the same interface?  Do we need to get their approval on this before implementing it in Gecko as well?

Comment 8

[Josh Aas]

Attachment: fix v1.0

The spec I proposed has problems. It would require calling NPP_SetValue with a null instance. It also does not allow different instances to have different private mode states, something browsers may wish to do in the future. I proposed this spec to resolve those issues:

=========================
Plugins should assume that private mode is off by default.

When the browser turns private mode on it will call NPP_SetValue for "NPPVprivateModeBool" with a non-null value for the argument pointer on all applicable instances. Plugins should check the boolean value of the pointer itself, not what it points to. When the browser turns private mode off it will call NPP_SetValue for "NPPVprivateModeBool" with a null value for the argument pointer on all applicable instances.

Plugins can query the browser for private mode state by calling NPN_GetValue for the variable "NPNVprivateModeBool". This allows instances to know about private mode state before opening any streams.

Changes in private mode affect new streams only - not previously created streams.
=========================

After further discussion about providing both polling capabilities and notification, I think the best thing to do is add the ability to ask about private mode state and add notification later if it becomes more clear that we want to do that. This way we can implement what we know is necessary right now.

This patch implements the following spec:

=========================
Plugins instances should assume that private mode is off by default.

Instances can query the browser for private mode state by calling NPN_GetValue for the variable "NPNVprivateModeBool". This allows instances to know about private mode state at any time.

Changes in private mode affect new streams only - not previously created streams.
=========================

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 358850 [details] [diff] [review]
fix v1.0

This looks good, but if for some reason we end up not fixing bug 475141 for Firefox 3.1, we'll need to push/pop a null context on the JS context stack around the call to GetPrivateBrowsingEnabled().

Josh, I think we should make your test plugin test this, and then request approval for the patch including a test.

Comment 10

[Josh Aas]

Attachment: fix v2.0

Query and notification implementation plus tests. Probably not quite ready yet.

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 359597 [details] [diff] [review]
fix v2.0

- In nsNPAPIPluginInstance::PrivateModeStateChanged():

+      PRBool pme = PR_FALSE;
+      pbs->GetPrivateBrowsingEnabled(&pme);

Check for errors here.

- In modules/plugin/test/mochitest/test_privatemode.xul:

+  try {
+    state1 = pluginElement1.queryPrivateModeState();
+    state2 = pluginElement2.queryPrivateModeState();

Should we test here that queryPrivateModeState() == lastReportedPrivateModeState() as well? You'd have to make the plugin fetch the current state on initialization for that to work tho.

+  ok(exceptionThrown == false, "Exception thrown getting private mode state.");

I hear "is(value, expected value, "...")" gives better error messages than "ok(value == expected value, "...")", so use that everywhere if you can.

+    state1 = pluginElement1.lastReportedPrivateModeState();
+    state2 = pluginElement2.lastReportedPrivateModeState();

fetch and compare current and last state here too?

Looks good! r+sr=jst, but please have a look at the above...

Comment 12

[Josh Aas]

Attachment: fix v2.1

Comment 13

[Josh Aas]

pushed v2.1 to mozilla-central

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

Comment 14

[Josh Aas]

Spec I implemented:

=========================
Plugins should assume that private mode is off by default.

When the browser turns private mode on it will call NPP_SetValue for "NPNVprivateModeBool" with a value of true on all applicable instances. When the browser turns private mode off it will call NPP_SetValue for "NPNVprivateModeBool" with a value of false on all applicable instances.

Plugins can query the browser for private mode state by calling NPN_GetValue
for the variable "NPPVprivateModeBool". This allows instances to know about
private mode state before opening any streams.

Changes in private mode affect new streams only - not previously created
streams.

This feature appears in NPAPI minor version 22.

"NPPVprivateModeBool = 19"
"NPNVprivateModeBool = 18"
=========================

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Josh, do we have a test plugin somewhere we could use to check for the correct behavior?

Comment 16

[Nochum Sossonko [:Natch]]

Henrik, this patch contains a test already that uses the test plugin from bug 386676.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thanks for the info. Based on the passed tests I can verify this enhancement on each platform:

*** 575 INFO Running chrome://mochikit/content/chrome/modules/plugin/test/test_privatemode.xul...
*** 576 INFO TEST-PASS | chrome://mochikit/content/chrome/modules/plugin/test/test_privatemode.xul | Exception thrown getting private mode state.
*** 577 INFO TEST-PASS | chrome://mochikit/content/chrome/modules/plugin/test/test_privatemode.xul | Browser returned incorrect private mode state.
*** 578 INFO TEST-PASS | chrome://mochikit/content/chrome/modules/plugin/test/test_privatemode.xul | Browser returned incorrect private mode state.
*** 579 INFO TEST-PASS | chrome://mochikit/content/chrome/modules/plugin/test/test_privatemode.xul | Exception thrown getting private mode state.
*** 580 INFO TEST-PASS | chrome://mochikit/content/chrome/modules/plugin/test/test_privatemode.xul | Private mode state reported incorrectly.
*** 581 INFO TEST-PASS | chrome://mochikit/content/chrome/modules/plugin/test/test_privatemode.xul | Private mode state reported incorrectly.

Comment 18

[Josh Aas]

I removed the unnecessary NPPVprivateModeBool enum.

http://hg.mozilla.org/mozilla-central/rev/02fe3defdfc5

Comment 19

[Josh Aas]

Attachment: fix v2.2 (1.9.1 branch)

Ported to 1.9.1 branch including a followup fix already landed on trunk.

Comment 20

[Josh Aas]

pushed to mozilla-1.9.1

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/32bfab65dfdc

Comment 21

[Eric Shepherd [:sheppy]]

This is on documentation radar; will see about getting this written up in the next day or two.

Comment 22

[Eric Shepherd [:sheppy]]

Added mentions of NPNVprivateModeBool where appropriate.  I don't see any other changes that are directly facing toward plugin devs, so marking this doc complete.  If I missed something re-mark it and let me know what I missed.  Thanks!