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

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Core
Plug-ins
VERIFIED FIXED
10 years ago
4 years ago

People

(Reporter: Ehsan, Assigned: Josh Aas)

Tracking

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

Trunk
mozilla1.9.2a1
dev-doc-complete, fixed1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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

10 years ago
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.

Comment 3

10 years ago
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?

Comment 4

10 years ago
Would if be possible for plugins to identify whether they supported PB mode, or could that be inferred from registering for a PB notification.
(Reporter)

Comment 5

10 years ago
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)

Updated

9 years ago
Assignee: nobody → joshmoz
(Assignee)

Comment 6

9 years ago
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.
(Reporter)

Comment 7

9 years ago
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?
(Assignee)

Comment 8

9 years ago
Created attachment 358850 [details] [diff] [review]
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.
=========================
Attachment #358850 - Flags: superreview?(jst)

Updated

9 years ago
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+
(Assignee)

Comment 10

9 years ago
Created attachment 359597 [details] [diff] [review]
fix v2.0

Query and notification implementation plus tests. Probably not quite ready yet.
Attachment #358850 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #359597 - Flags: superreview?(jst)

Updated

9 years ago
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...
(Assignee)

Comment 12

9 years ago
Created attachment 359716 [details] [diff] [review]
fix v2.1
Attachment #359597 - Attachment is obsolete: true
(Assignee)

Comment 13

9 years ago
pushed v2.1 to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/ff75b28255c9
(Assignee)

Updated

9 years ago
Flags: wanted1.9.1? → blocking1.9.1?
(Assignee)

Comment 14

9 years ago
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
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
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
(Assignee)

Comment 18

9 years ago
I removed the unnecessary NPPVprivateModeBool enum.

http://hg.mozilla.org/mozilla-central/rev/02fe3defdfc5
Depends on: 476406
Flags: blocking1.9.1? → blocking1.9.1+
(Assignee)

Updated

9 years ago
Attachment #359716 - Flags: approval1.9.1?
(Assignee)

Comment 19

9 years ago
Created attachment 360643 [details] [diff] [review]
fix v2.2 (1.9.1 branch)

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

Comment 20

9 years ago
pushed to mozilla-1.9.1

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

Updated

9 years ago
Depends on: 477918
Depends on: 386676
(Reporter)

Updated

9 years ago
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!
Keywords: dev-doc-needed → dev-doc-complete
Attachment #360643 - Attachment is patch: true
Attachment #360643 - Attachment mime type: application/octet-stream → text/plain
(Reporter)

Updated

8 years ago
Duplicate of this bug: 563572
You need to log in before you can comment on or make changes to this bug.