Closed Bug 722976 Opened 8 years ago Closed 8 years ago

ConsoleAPIStorage uses global Private Browsing status to make decisions

Categories

(DevTools :: Console, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 16

People

(Reporter: jdm, Assigned: cwiiis)

References

Details

Attachments

(1 file, 8 obsolete files)

The global PB service is going away. The docshell for the relevant window should be queried instead.
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.
Attached patch v1 (obsolete) — Splinter Review
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
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?
Sure.
Attached patch v2 (obsolete) — Splinter Review
all mochitest-browser-chrome tests passed.
Attachment #617641 - Attachment is obsolete: true
Attachment #617858 - Flags: review?
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.
Attached patch v3 (obsolete) — Splinter Review
Attachment #617858 - Attachment is obsolete: true
Attachment #617858 - Flags: review?
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.
Attached patch v4 (obsolete) — Splinter Review
Attachment #617879 - Attachment is obsolete: true
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!)
Hessam, do you have the time to update your patch according to gavin's feedback?
Assignee: hessamms → chrislord.net
Attached patch v5 (obsolete) — Splinter Review
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)
Attached patch v5.5 (obsolete) — Splinter Review
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+
(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.
Depends on: 769467
Attached patch v6 (obsolete) — Splinter Review
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)
(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...
(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.
Attached patch v7 (obsolete) — Splinter Review
I am SUCH a chump sometimes.
Attachment #638417 - Attachment is obsolete: true
Attachment #638417 - Flags: feedback?(gavin.sharp)
Attachment #638690 - Flags: review?(gavin.sharp)
OS: Mac OS X → All
Hardware: x86 → All
Attached patch v7.5Splinter Review
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+
(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.
https://hg.mozilla.org/mozilla-central/rev/7747e6a69ebe
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.