Remove nsIPrivacyTransitionObserver

NEW
Unassigned

Status

()

Firefox
Private Browsing
5 years ago
3 years ago

People

(Reporter: Away for a while, Unassigned, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=c++])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
I can't think of any reason why we would need to keep supporting this interface.  Can you, Josh?
(Reporter)

Comment 1

5 years ago
nsIPrivacyTransitionObserver should be pretty useless these days, since we only create docshells with the correct PB status in the first place.  Is that right, Josh?

Comment 2

5 years ago
Agreed. Let's throw it away.
Whiteboard: [mentor=jdm][lang=c++]
(Reporter)

Comment 3

5 years ago
I'm gonna unblock on this, actually.  We don't need this to ship pbngen.
No longer blocks: 463027

Comment 4

5 years ago
Created attachment 685077 [details] [diff] [review]
Patch

Not sure if my changes to nsDOMStorage and StorageChild are correct.
Assignee: nobody → ewong3
Status: NEW → ASSIGNED
Attachment #685077 - Flags: feedback?(josh)

Comment 5

5 years ago
Comment on attachment 685077 [details] [diff] [review]
Patch

Great work, thanks! The StorageChild needs to be fixed up, as you suspected, and we can remove the nsNPAPIPluginInstance::PivateModeStateChanged function entirely, since we're getting rid of the only caller.

>[scriptable, builtinclass, uuid(318CE516-3F7A-41F6-8F3D-3661650F7A46)]
>interface nsIDocShell : nsISupports

This iid will need need to be changed, since the interface is being modified.

>diff --git a/dom/base/nsGlobalWindow.cpp b/dom/base/nsGlobalWindow.cpp
> 
>-    if (mLocalStorage) {

Nit: get rid of the extraneous newline right here.

> 
>-    nsCOMPtr<nsIPrivacyTransitionObserver> obs = do_GetInterface(mSessionStorage);

Likewise here.

>                                                      getter_AddRefs(mLocalStorage));
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>-    nsCOMPtr<nsIPrivacyTransitionObserver> obs = do_GetInterface(mLocalStorage);

Likewise here.

>diff --git a/dom/plugins/base/nsPluginInstanceOwner.cpp b/dom/plugins/base/nsPluginInstanceOwner.cpp
>   mInstance = aInstance;
> 
>-  nsCOMPtr<nsIDocument> doc;

Likewise here.

>diff --git a/dom/src/storage/PStorage.ipdl b/dom/src/storage/PStorage.ipdl
>       returns (nsresult rv);
> 
>-  UpdatePrivateState(bool enabled);

Likewise here.

>diff --git a/dom/src/storage/StorageChild.cpp b/dom/src/storage/StorageChild.cpp
> NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(StorageChild)
>-  NS_INTERFACE_MAP_ENTRY(nsIPrivacyTransitionObserver)
>+  NS_INTERFACE_MAP_ENTRY(DOMStorageBase)
>   NS_INTERFACE_MAP_ENTRY(nsISupportsWeakReference)
>-  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIPrivacyTransitionObserver)
>+  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, DOMStorageBase)
> NS_INTERFACE_MAP_END

As you suspected, this isn't quite right. I think the best thing to do right now is make DOMStorageBase inherit from nsISupports, as you did, and get rid of StorageChild's inheritance from nsSupportsWeakReference, at which point the only MAP_ENTRY line you need is one for nsISupports. The same changes can be made for DOMStorageImpl as well.

>diff --git a/dom/src/storage/StorageChild.h b/dom/src/storage/StorageChild.h
>+  NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS(StorageChild, DOMStorageBase)

This won't need to be ambiguous after the previous changes are made.
Attachment #685077 - Flags: feedback?(josh) → feedback+

Comment 6

5 years ago
We should also check whether this breaks any tests; in particular, dom/plugins/test/mochitest/test_privatemode.xul and browser/components/privatebrowsing/test/browser/global/browser_privatebrowsing_localStorage_before_after.js. These can be run from the mochitest-chrome and mochitest-browser-chrome test suites respectively.

Comment 7

5 years ago
Created attachment 685987 [details] [diff] [review]
Patch

Made changes from feedback in Comment 5.
Also removed test: docshell/test/unit/test_privacy_transition.js.

Ran tests suggested in Comment 6 -
"browser_privatebrowsing_localStorage_before_after.js" passes.
"test_privatemode.xul" fails 4/12 tests (it passes 12/12 without this patch).

Tests fail because after executing "privateBrowsing.privateBrowsingEnabled = true;" in http://mxr.mozilla.org/mozilla-central/source/dom/plugins/test/mochitest/test_privatemode.xul#59, calls to lastReportedPrivateModeState() on lines 62 and 63 return false, so tests on lines 68, 69, 79, 80 fail.

Given the nature of this bug, it's entirely possible that I removed code such that some privacy state is no longer properly maintained.

I traced "privateBrowsing.privateBrowsingEnabled = true" to nsPrivateBrowsingServiceWrapper::SetPrivateBrowsingEnabled() here: http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingServiceWrapper.cpp#62

mPBService = do_GetService("@mozilla.org/privatebrowsing;1", &rv) 
and I don't know where to look after this.  I get turned around in circles trying to trace this do_GetService() call.

Josh, if you can tell me what mPBService here ultimately points to, I want to see if it leads to code that I removed in this patch that cause the tests to fail.
Then I suppose we can determine if these tests are still valid or do I need to put some of the code back or maybe nsIPrivacyTransitionObserver is still needed after all?

(BTW, the Platform for this bug is set to x86 Mac OS X.  Is that significant for this bug?)
Attachment #685077 - Attachment is obsolete: true
Flags: needinfo?(josh)

Comment 8

5 years ago
You're looking for http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js, but I can tell you what's happening here. We're removing the code for plugins that watches for privacy updates within a particular docshell (basically tab or window, for sake of explanation) and notifies each plugin instance of the change. This is technically ok, because we have decided that we're not going to support switching the privacy status of tabs after their initial creation, however, it does break the nature of the test. The proper thing to do here would be to do something like
* open a new tab containing the plugin, perform the tests for non-private mode
* close the tab, switch to private mode
* open a new tab containing the plugin, perform the tests for private mode
* close the tab, switch out of private mode
* open a new tab containing the plugin, perform the remaining tests for non-private mode

This will necessitate making the test a mochitest-browser-chrome one. You should be able to get away with creating a browser_plugin_privatebrowsing.js test that loads xul files in new tabs. Since no browser-chrome tests exist for dom/plugins/test right now, you can crib examples from http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/newtab/Makefile.in and http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_bug763468.js?force=1
Flags: needinfo?(josh)
OS: Mac OS X → All
Hardware: x86 → All

Comment 9

5 years ago
Created attachment 687417 [details]
Testcase (still in work)

I created this new mochitest-browser-chrome test case, basically splitting up the existing mochitest-chrome testcase (http://mxr.mozilla.org/mozilla-central/source/dom/plugins/test/mochitest/test_privatemode.xul) to work in new tabs as described in Comment 8.  I get the same failures as described in Comment 7 though.  After enabling private browsing and opening a new tab, calling pluginElement.lastReportedPrivateModeState() returns false.

lastReportedPrivateModeState() method seems to be a test artifact (http://mxr.mozilla.org/mozilla-central/source/dom/plugins/test/testplugin/nptest.cpp), not main browser code.  I'm looking to see if changes are needed in nptest.cpp, but nothing obvious stands out to me on first pass and could use some help/hints.

The XUL document I'm loading into tabs is this: http://pastebin.mozilla.org/1970218
It's pretty much the original test_privatemode.xul testcase with all the javascript stripped out.
Flags: needinfo?(josh)

Updated

5 years ago
Attachment #687417 - Attachment mime type: application/x-javascript → text/plain

Comment 10

5 years ago
I found this: http://mxr.mozilla.org/mozilla-central/source/dom/plugins/test/testplugin/README?force=1#162

The setting of NPNVprivateModeBool occurs in nsNPAPIPluginInstance::PrivateModeStateChanged(), which was removed.

I think NPNVprivateModeBool needs to be set somewhere with the initial private browsing setting.
Flags: needinfo?(josh)

Comment 11

5 years ago
Thinking of adding this code somewhere in nsNPAPIPluginInstance to initialize NPNVprivateModeBool properly, perhaps in nsNPAPIPluginInstance::Initialize():

nsCOMPtr<nsIPrivateBrowsingService> pbs = do_GetService(NS_PRIVATE_BROWSING_SERVICE_CONTRACTID);
if (pbs) {
  bool inPrivateBrowsing = false;
  pbs->GetPrivateBrowsingEnabled(&inPrivateBrowsing);
  NPError error;
  NPPluginFuncs* pluginFunctions = mPlugin->PluginFuncs();
  NPBool value = static_cast<NPBool>(inPrivateBrowsing);
  NS_TRY_SAFE_CALL_RETURN(error, (*pluginFunctions->setvalue)(&mNPP, NPNVprivateModeBool, &value), this);
}

Will give it a try and see, but please let me know if I'm on the right track, or if lastReportedPrivateModeState might not be needed anymore in test plugin.
You're on the right track, I believe, but we shouldn't add more uses of nsIPrivateBrowsingService, since it doesn't understand per-window private browsing. Instead, do something like this:

>bool pbEnabled = false;
>nsCOMPtr<nsPIDOMWindow> win = GetDOMWindow();
>if (win) {
>  nsCOMPtr<nsIDocShell> docShell = win->GetDocShell();
>  nsCOMPtr<nsILoadContext> loadContext = do_QueryInterface(docShell);
>  pbEnabled = loadContext && loadContext->UsePrivateBrowsing();
>}

We'll want to make sure that we call GetDOMWindow from somewhere where the window is actually obtainable; it looks like after mOwner is set it should be ok, so give Initialize a shot.
Hi Eric, are you still working on this bug? Just let us know!  Thanks!
Flags: needinfo?(ewong3)

Comment 14

5 years ago
No, I'm not working on this bug.  I'm unassigning myself.
Assignee: ewong3 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(ewong3)
Hi, 

I am willing to work on this bug. So please I request you to assign it to me.

Thanks in Advance,

Regards,
A. Anup
(Assignee)

Updated

4 years ago
Mentor: josh@joshmatthews.net
Whiteboard: [mentor=jdm][lang=c++] → [lang=c++]

Updated

3 years ago
Assignee: nobody → allamsetty.anup

Updated

3 years ago
Assignee: allamsetty.anup → nobody
You need to log in before you can comment on or make changes to this bug.