ConsoleAPIStorage uses global Private Browsing status to make decisions

RESOLVED FIXED in Firefox 16

Status

()

Firefox
Developer Tools: Console
P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jdm, Assigned: cwiiis)

Tracking

unspecified
Firefox 16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

6 years ago
The global PB service is going away. The docshell for the relevant window should be queried instead.

Updated

6 years ago
Priority: -- → P2
ConsoleAPI.init <http://mxr.mozilla.org/mozilla-central/source/dom/base/ConsoleAPI.js#61> takes an nsIDOMWindow parameter called aWindow.  If you port the helper function _getBrowserWindowForContentWindow from bug 722986, you can get access to the privateWindow property of the browser window corresponding to aWindow, similar to what you did in bug 722986.

When you have the privateWindow flag, you can pass it to notifyObservers calls on line 82 and below as an additional argument (let's call it aPrivateWindow).  In notifyObservers, we call ConsoleAPIStorage.recordEvent <http://mxr.mozilla.org/mozilla-central/source/dom/base/ConsoleAPI.js#211>.  Now, you can avoid calling recordEvent if aPrivateWindow is true.

recordEvent is defined here: <http://mxr.mozilla.org/mozilla-central/source/dom/base/ConsoleAPIStorage.jsm#135>.  It currently uses the global private window API (gPrivBrowsing and its usages).  Given what you've done in the last paragraph, recordEvent will not be called any more, so you can get rid of all of the gPrivBrowsing occurrences in that file.

This test is testing the code in question here: <http://mxr.mozilla.org/mozilla-central/source/dom/tests/browser/browser_ConsoleStoragePBTest.js>.  In order to verify that your patches do not break anything, you can run the tests in this directory using this command:

make -C objdir mochitest-browser-chrome TEST_PATH=dom/

where "objdir" is the name of your object directory.

Comment 2

5 years ago
Created attachment 617641 [details] [diff] [review]
v1

I get the following error when I try mochitest and I can't find what I did worng ?

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/dom/tests/browser/browser_ConsoleStoragePBTest.js | storage shouldn't occur - Got true, expected false
The problem with this patch is that you assign to the privateWindow object in the init() method which only gets called the first time the object is initialized, and then you use that value in all future calls to recordEvent.  So, if privateWindow is false initially (which is the case here), you will end up recording all of the events.

A good way to fix this is to add a function which takes aWindow as a parameter and returns the privateWindow property for that window (let's call the function isWindowPrivate(aWindow) for example), and then call that and pass the result as the fourth argument to recordEvent in all of the console member functions.
Assignee: nobody → hessamms
(Reporter)

Comment 4

5 years ago
In the event that only a single tab is in private mode, should we be using the value of the closest docshell instead of the whole window?
(In reply to Josh Matthews [:jdm] from comment #4)
> In the event that only a single tab is in private mode, should we be using
> the value of the closest docshell instead of the whole window?

Maybe.  I was actually thinking of making .privateWindow return the private status of the current tab.  But I still haven't made up my mind on whether that's a good idea or not.  Let's proceed with using privateWindow for now, and we can decide later (we can always change all of its users quite easily.)  Do you agree?
(Reporter)

Comment 6

5 years ago
Sure.

Comment 7

5 years ago
Created attachment 617858 [details] [diff] [review]
v2

all mochitest-browser-chrome tests passed.
Attachment #617641 - Attachment is obsolete: true
Attachment #617858 - Flags: review?
(Reporter)

Comment 8

5 years ago
Comment on attachment 617858 [details] [diff] [review]
v2

Thanks Hessam! However, looking at all the repeated self.isWindowPrivate calls, I think it would be clearer to just pass aWindow to notifyObservers and then check this.isWindowPrivate(aWindow) inside of the method.
Or even just continue to have recordEvent() do the check, by getting the window from the window ID and checking it's private browsing state.

Comment 10

5 years ago
Created attachment 617879 [details] [diff] [review]
v3
Attachment #617858 - Attachment is obsolete: true
Attachment #617858 - Flags: review?
(Reporter)

Comment 11

5 years ago
Comment on attachment 617879 [details] [diff] [review]
v3

One more thing, and I'm sorry I didn't point this out before - please indent the recordEvent call, and make the closing } line up with the if statement. If you can also follow the instructions at https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in to add a commit message and your user information (name and email) to this patch, it will be easier to check in when it's finished.

Comment 12

5 years ago
Created attachment 617891 [details] [diff] [review]
v4
Attachment #617879 - Attachment is obsolete: true
(Reporter)

Comment 13

5 years ago
Comment on attachment 617891 [details] [diff] [review]
v4

Thank you Hessam!
Attachment #617891 - Flags: review?(gavin.sharp)
Comment on attachment 617891 [details] [diff] [review]
v4

>diff --git a/dom/base/ConsoleAPI.js b/dom/base/ConsoleAPI.js

>+        self.notifyObservers(outerID, innerID, "log", self.processArguments(arguments), aWindow);

If you're going to start passing the window object, you can move the ID-getting code into notifyObservers and avoid passing the IDs.

>+  isWindowPrivate: function CA_isWindowPrivate(aWindow) {

>+    return browserWindow.gPrivateBrowsingUI.privateWindow;

You can't assume that all windows where these events are triggered are Firefox browser windows (with a gPrivateBrowsingUI property). But you can easily just check nsILoadContext.usePrivateBrowsing directly.
Attachment #617891 - Flags: review?(gavin.sharp) → review-
(and sorry for the delayed review request response!)
(Reporter)

Comment 16

5 years ago
Hessam, do you have the time to update your patch according to gavin's feedback?
Assignee: hessamms → chrislord.net
(Assignee)

Comment 17

5 years ago
Created attachment 637858 [details] [diff] [review]
v5

Rebased and comments addressed - I passed the private-browsing flag state, rather than the window itself. It seems passing the window, given underlying changes, may cause a reference to hang around (otherwise the check for whether the window has been deleted would be pointless?)

Of course, I'm likely wrong, I don't really know this part of the code yet.
Attachment #617891 - Attachment is obsolete: true
Attachment #637858 - Flags: review?(gavin.sharp)
(Assignee)

Comment 18

5 years ago
Created attachment 637859 [details] [diff] [review]
v5.5

Sorry, slightly botched rebase. Fixed.
Attachment #637858 - Attachment is obsolete: true
Attachment #637858 - Flags: review?(gavin.sharp)
Attachment #637859 - Flags: review?(gavin.sharp)
Comment on attachment 637859 [details] [diff] [review]
v5.5

Yeah, this is probably better.

isWindowPrivate can be replaced by a call to the method being added in bug 769467 once it exists, right?
Attachment #637859 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 20

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #19)
> Comment on attachment 637859 [details] [diff] [review]
> v5.5
> 
> Yeah, this is probably better.
> 
> isWindowPrivate can be replaced by a call to the method being added in bug
> 769467 once it exists, right?

Right, I'll have a look at that next before adding more redundant code.
(Assignee)

Updated

5 years ago
Depends on: 769467
(Assignee)

Comment 21

5 years ago
Created attachment 638417 [details] [diff] [review]
v6

Updated to use PrivateBrowsingUtils added in bug 769467.

I also realise I didn't fix the issue described in comment #3 - I tried to do that with this patch, but I still get a failure where storage should occur but doesn't.

Any ideas?
Attachment #637859 - Attachment is obsolete: true
Attachment #638417 - Flags: feedback?(gavin.sharp)
(Assignee)

Comment 22

5 years ago
(In reply to Chris Lord [:cwiiis] from comment #21)
> Created attachment 638417 [details] [diff] [review]
> v6
> 
> Updated to use PrivateBrowsingUtils added in bug 769467.
> 
> I also realise I didn't fix the issue described in comment #3 - I tried to
> do that with this patch, but I still get a failure where storage should
> occur but doesn't.
> 
> Any ideas?

The problem is that PrivateBrowsingUtils fails... Will sort it out...
(Assignee)

Comment 23

5 years ago
(In reply to Chris Lord [:cwiiis] from comment #22)
> (In reply to Chris Lord [:cwiiis] from comment #21)
> > Created attachment 638417 [details] [diff] [review]
> > v6
> > 
> > Updated to use PrivateBrowsingUtils added in bug 769467.
> > 
> > I also realise I didn't fix the issue described in comment #3 - I tried to
> > do that with this patch, but I still get a failure where storage should
> > occur but doesn't.
> > 
> > Any ideas?
> 
> The problem is that PrivateBrowsingUtils fails... Will sort it out...

Sorry, was wrong, PrivateBrowsingUtils is fine - don't know why this occurs yet.
(Assignee)

Comment 24

5 years ago
Created attachment 638690 [details] [diff] [review]
v7

I am SUCH a chump sometimes.
Attachment #638417 - Attachment is obsolete: true
Attachment #638417 - Flags: feedback?(gavin.sharp)
Attachment #638690 - Flags: review?(gavin.sharp)

Updated

5 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 25

5 years ago
Created attachment 638707 [details] [diff] [review]
v7.5

Adapt to change in blocking bug.
Attachment #638690 - Attachment is obsolete: true
Attachment #638690 - Flags: review?(gavin.sharp)
Attachment #638707 - Flags: review?(gavin.sharp)
Comment on attachment 638707 [details] [diff] [review]
v7.5

>diff --git a/dom/base/ConsoleAPI.js b/dom/base/ConsoleAPI.js

>+  queueCall: function CA_queueCall(aMethod, aArguments, aWindow)

>+    try {
>+      let windowUtils = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>+                          .getInterface(Ci.nsIDOMWindowUtils);
>+
>+      outerID = windowUtils.outerWindowID;
>+      innerID = windowUtils.currentInnerWindowID;
>+      isPrivate = PrivateBrowsingUtils.isWindowPrivate(aWindow);
>+    }
>+    catch (ex) {
>+      Cu.reportError(ex);
>+    }

This is not really related to this patch, but it seems like if we fail to get the ID, we should avoid trying to report anything at all (and return in addition to calling reportError). I don't see how that would ever happen in practice, though. The isPrivate call can live outside the try/catch block, anyhow.

>     let metaForCall = {
>-      outerID: aMeta.outerID,
>+      ID: outerID,

This seems like an unnecessary change (and confusing, since the other "meta" objects still use "outerID").

r=me with those addressed.
Attachment #638707 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 27

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #26)
> Comment on attachment 638707 [details] [diff] [review]
> v7.5
> 
> >diff --git a/dom/base/ConsoleAPI.js b/dom/base/ConsoleAPI.js
> 
> >+  queueCall: function CA_queueCall(aMethod, aArguments, aWindow)
> 
> >+    try {
> >+      let windowUtils = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> >+                          .getInterface(Ci.nsIDOMWindowUtils);
> >+
> >+      outerID = windowUtils.outerWindowID;
> >+      innerID = windowUtils.currentInnerWindowID;
> >+      isPrivate = PrivateBrowsingUtils.isWindowPrivate(aWindow);
> >+    }
> >+    catch (ex) {
> >+      Cu.reportError(ex);
> >+    }
> 
> This is not really related to this patch, but it seems like if we fail to
> get the ID, we should avoid trying to report anything at all (and return in
> addition to calling reportError). I don't see how that would ever happen in
> practice, though. The isPrivate call can live outside the try/catch block,
> anyhow.

Will move isPrivate out. Will add the return too.

> >     let metaForCall = {
> >-      outerID: aMeta.outerID,
> >+      ID: outerID,
> 
> This seems like an unnecessary change (and confusing, since the other "meta"
> objects still use "outerID").
> 
> r=me with those addressed.

Right, unintentional, will fix.
(Assignee)

Comment 28

5 years ago
Changes made, pushed to inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/7747e6a69ebe
https://hg.mozilla.org/mozilla-central/rev/7747e6a69ebe
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
https://hg.mozilla.org/mozilla-central/rev/7747e6a69ebe
You need to log in before you can comment on or make changes to this bug.