Last Comment Bug 722976 - ConsoleAPIStorage uses global Private Browsing status to make decisions
: ConsoleAPIStorage uses global Private Browsing status to make decisions
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: Firefox 16
Assigned To: Chris Lord [:cwiiis]
:
Mentors:
Depends on: 722840 769467
Blocks: PBnGen
  Show dependency treegraph
 
Reported: 2012-01-31 21:32 PST by Josh Matthews [:jdm]
Modified: 2012-07-04 14:04 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (6.77 KB, patch)
2012-04-23 14:12 PDT, Hessam Salehi
no flags Details | Diff | Review
v2 (7.17 KB, patch)
2012-04-24 07:21 PDT, Hessam Salehi
no flags Details | Diff | Review
v3 (6.92 KB, patch)
2012-04-24 08:28 PDT, Hessam Salehi
no flags Details | Diff | Review
v4 (5.48 KB, patch)
2012-04-24 09:01 PDT, Hessam Salehi
gavin.sharp: review-
Details | Diff | Review
v5 (3.73 KB, patch)
2012-06-29 05:13 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Review
v5.5 (4.57 KB, patch)
2012-06-29 05:16 PDT, Chris Lord [:cwiiis]
gavin.sharp: review+
Details | Diff | Review
v6 (9.16 KB, patch)
2012-07-02 11:02 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Review
v7 (9.01 KB, patch)
2012-07-03 06:45 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Review
v7.5 (9.02 KB, patch)
2012-07-03 07:18 PDT, Chris Lord [:cwiiis]
gavin.sharp: review+
Details | Diff | Review

Description Josh Matthews [:jdm] 2012-01-31 21:32:08 PST
The global PB service is going away. The docshell for the relevant window should be queried instead.
Comment 1 :Ehsan Akhgari (out sick) 2012-04-16 15:46:21 PDT
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 Hessam Salehi 2012-04-23 14:12:19 PDT
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
Comment 3 :Ehsan Akhgari (out sick) 2012-04-23 15:26:09 PDT
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.
Comment 4 Josh Matthews [:jdm] 2012-04-23 18:45:52 PDT
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?
Comment 5 :Ehsan Akhgari (out sick) 2012-04-23 18:52:37 PDT
(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?
Comment 6 Josh Matthews [:jdm] 2012-04-23 18:55:13 PDT
Sure.
Comment 7 Hessam Salehi 2012-04-24 07:21:41 PDT
Created attachment 617858 [details] [diff] [review]
v2

all mochitest-browser-chrome tests passed.
Comment 8 Josh Matthews [:jdm] 2012-04-24 07:31:36 PDT
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.
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-04-24 07:38:11 PDT
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 Hessam Salehi 2012-04-24 08:28:44 PDT
Created attachment 617879 [details] [diff] [review]
v3
Comment 11 Josh Matthews [:jdm] 2012-04-24 08:33:23 PDT
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 Hessam Salehi 2012-04-24 09:01:28 PDT
Created attachment 617891 [details] [diff] [review]
v4
Comment 13 Josh Matthews [:jdm] 2012-04-24 09:06:23 PDT
Comment on attachment 617891 [details] [diff] [review]
v4

Thank you Hessam!
Comment 14 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-10 11:30:20 PDT
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.
Comment 15 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-10 11:31:24 PDT
(and sorry for the delayed review request response!)
Comment 16 Josh Matthews [:jdm] 2012-05-25 07:43:41 PDT
Hessam, do you have the time to update your patch according to gavin's feedback?
Comment 17 Chris Lord [:cwiiis] 2012-06-29 05:13:55 PDT
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.
Comment 18 Chris Lord [:cwiiis] 2012-06-29 05:16:41 PDT
Created attachment 637859 [details] [diff] [review]
v5.5

Sorry, slightly botched rebase. Fixed.
Comment 19 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-29 05:27:10 PDT
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?
Comment 20 Chris Lord [:cwiiis] 2012-06-29 05:34:42 PDT
(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.
Comment 21 Chris Lord [:cwiiis] 2012-07-02 11:02:15 PDT
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?
Comment 22 Chris Lord [:cwiiis] 2012-07-03 06:20:27 PDT
(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...
Comment 23 Chris Lord [:cwiiis] 2012-07-03 06:35:13 PDT
(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.
Comment 24 Chris Lord [:cwiiis] 2012-07-03 06:45:45 PDT
Created attachment 638690 [details] [diff] [review]
v7

I am SUCH a chump sometimes.
Comment 25 Chris Lord [:cwiiis] 2012-07-03 07:18:02 PDT
Created attachment 638707 [details] [diff] [review]
v7.5

Adapt to change in blocking bug.
Comment 26 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-03 09:28:10 PDT
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.
Comment 27 Chris Lord [:cwiiis] 2012-07-03 11:44:34 PDT
(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.
Comment 28 Chris Lord [:cwiiis] 2012-07-04 08:10:03 PDT
Changes made, pushed to inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/7747e6a69ebe
Comment 29 :Ehsan Akhgari (out sick) 2012-07-04 14:04:00 PDT
https://hg.mozilla.org/mozilla-central/rev/7747e6a69ebe
Comment 30 :Ehsan Akhgari (out sick) 2012-07-04 14:04:02 PDT
https://hg.mozilla.org/mozilla-central/rev/7747e6a69ebe

Note You need to log in before you can comment on or make changes to this bug.