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

VERIFIED FIXED in mozilla1.9.2a1

Status

()

defect
VERIFIED FIXED
11 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: jaas)

Tracking

({dev-doc-complete, fixed1.9.1})

Trunk
mozilla1.9.2a1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(2 attachments, 2 obsolete attachments)

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.
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?
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.
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.
Flags: wanted1.9.1?
Would if be possible for plugins to identify whether they supported PB mode, or could that be inferred from registering for a PB notification.
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.
Assignee: nobody → joshmoz
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.
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?
Posted patch fix v1.0 (obsolete) — Splinter Review
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.
=========================
Attachment #358850 - Flags: superreview?(jst)
Depends on: 475141
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.
Attachment #358850 - Flags: superreview?(jst)
Attachment #358850 - Flags: superreview+
Attachment #358850 - Flags: review+
Posted patch fix v2.0 (obsolete) — Splinter Review
Query and notification implementation plus tests. Probably not quite ready yet.
Attachment #358850 - Attachment is obsolete: true
Attachment #359597 - Flags: superreview?(jst)
Attachment #359597 - Flags: superreview?(jst)
Attachment #359597 - Flags: superreview+
Attachment #359597 - Flags: review+
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...
Posted patch fix v2.1Splinter Review
Attachment #359597 - Attachment is obsolete: true
pushed v2.1 to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/ff75b28255c9
Flags: wanted1.9.1? → blocking1.9.1?
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"
=========================
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Attachment #359716 - Flags: approval1.9.1?
Josh, do we have a test plugin somewhere we could use to check for the correct behavior?
Henrik, this patch contains a test already that uses the test plugin from bug 386676.
Flags: in-testsuite+
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.
Status: RESOLVED → VERIFIED
Target Milestone: --- → mozilla1.9.2a1
I removed the unnecessary NPPVprivateModeBool enum.

http://hg.mozilla.org/mozilla-central/rev/02fe3defdfc5
Depends on: 476406
Flags: blocking1.9.1? → blocking1.9.1+
Attachment #359716 - Flags: approval1.9.1?
Ported to 1.9.1 branch including a followup fix already landed on trunk.
pushed to mozilla-1.9.1

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/32bfab65dfdc
Keywords: fixed1.9.1
Keywords: dev-doc-needed
This is on documentation radar; will see about getting this written up in the next day or two.
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!
Attachment #360643 - Attachment is patch: true
Attachment #360643 - Attachment mime type: application/octet-stream → text/plain
Duplicate of this bug: 563572
You need to log in before you can comment on or make changes to this bug.