722988 - openLocationLastURL.jsm uses global Private Browsing state to make decisions

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Josh Matthews [:jdm]]

The global state is going away. Ideally this would check a docshell instead; we might need to modify the interface to require one as a parameter.

Comment 1

[Josh Matthews [:jdm]]

What we should do here is convert gOpenLocationLastURL (http://mxr.mozilla.org/mozilla-central/source/browser/modules/openLocationLastURL.jsm#41) from a singleton to an instance that takes a window. Ie. from let gOpenLocationLastURL = { ... } to
>function OpenLocationLastURL(window) {
>  this._window = window;
>}
>
>OpenLocationLastURL.prototype = {
> ...
>};

Duplicate the method at http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6426 in the JSM, then we can change the check of the service to check the gPrivateBrowsingUI.privateWindow of the browser window for the stored this._window variable.

Comment 2

[Josh Matthews [:jdm]]

Then we need to update consumers to do more than just import the JSM: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/openLocation.js#17

Instead, create a blank variable, import the JSM onto the variable, and create gOpenLocationLastURL by using |var gOpenLocationLastURL = new tmp.OpenLocationLastURL(window);|

Comment 3

[Saurabh Anand [:sawrubh]]

Attachment: Patch v1

A race for whoever first gives the feedback on the patch ;)

Comment 4

[Atul Jangra [:atuljangra]]

@saurabhanandiit I was working on this bug :)and I guess I've mentioned it on irc.  Josh Matthews was not around so I was not able to assign it to myself. Anyways, I'll pick on some other bug. 
I would suggest you to look into some slightly difficult bugs and leave these mentored bugs for newbies like me ;)
Now I am working on Bug 722995. I hope we are clear on it. :-)
Thanks and Regards

Comment 5

[(no longer active)]

Atul, sorry for the confusion here.  :-)  Please let me know if you want me to help you find another bug to work on.  Thanks!

Comment 6

[(no longer active)]

Comment on attachment 633645 [details] [diff] [review]
Patch v1

Review of attachment 633645 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good with the following comment addressed.  I can review a version which addresses it.

Have you submitted this patch to the try server yet?

::: browser/modules/openLocationLastURL.jsm
@@ +42,5 @@
> +  this._window = aWindow;
> +}
> +
> +OpenLocationLastURL.prototype = {
> +  isPrivate: function OpenLocationLastURL_isPrivate(aWindow) {

You can just access this._window here, no need to pass the member variable as an argument here.

Comment 7

[Saurabh Anand [:sawrubh]]

Attachment: Patch v2

Comment 8

[Saurabh Anand [:sawrubh]]

https://tbpl.mozilla.org/?tree=Try&rev=b2e6b3346bb1

Comment 9

[Saurabh Anand [:sawrubh]]

Attachment: Patch v3

The tests are passing locally for this patch. Let's see what Try feels about it.

Comment 10

[Saurabh Anand [:sawrubh]]

https://tbpl.mozilla.org/?tree=Try&rev=49190b75ab90

Comment 11

[Mozilla RelEng Bot]

Try run for 49190b75ab90 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=49190b75ab90
Results (out of 109 total builds):
    success: 70
    warnings: 15
    failure: 24
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-49190b75ab90

Comment 12

[Saurabh Anand [:sawrubh]]

So I found out the source of the problems and have found the fix also. The xpcshell tests which were failing are now passing locally on my machine. So I'll push to try one final time and hope this time it passes all the tests.

However I'm faced with a issue/problem. Now that we are faking the "window" for the xpcshell test, we will need to change |isPrivate()| function, too since in the case of this fake window, the window object doesn't have all the interfaces that we query to get the chrome window. So the workaround that I chose was that I added a flag "fake" to this fake window (See the patch for details) and check this flag in |isPrivate()| and return the values accordingly. Now this leads to another set of problems. When in |test_openLocationLastURL.js| there are calls like |pb.privateBrowsingEnabled = false|, they expect the PB flag for the fake window will be updated but they are not.

So the solution could be that I either remove these calls completely, since anyway with the fake window and everything these later tests become useless. Or I change these |pb.privateBrowsingEnabled = false| calls to something like |window.gPrivateBrowsingUI.privateWindow = false| where the "window" refers to the fake window, because this is the only way the PB flag of the fake window can be updated.

Any ideas guys ?

Comment 13

[Josh Matthews [:jdm]]

My suggestion is to create a function switchPrivateBrowsing(enabled) that both sets the private browsing service state and updates the singleton flag. I think the test is still valid even with all of the fakery. With regards to your fake flag, I think we can do a bit better: check |if (!("gPrivateBrowsingUI" in window))| and assign the QIed chrome window to the window variable, then return the regular value. Much simpler and clearer that way.

Comment 14

[Saurabh Anand [:sawrubh]]

Attachment: Patch v4

Fixed the xpcshell test.

Comment 15

[Saurabh Anand [:sawrubh]]

https://tbpl.mozilla.org/?tree=Try&rev=beead8a9336b

Comment 16

[Saurabh Anand [:sawrubh]]

The Try run looks very green. If you are not able to load the TBPL results page directly, then please go to https://tbpl.mozilla.org/?tree=Try ,and scroll down till my Try run details. There is a bug on file about this problem with TBPL.

@Josh, @Ehsan what do you think about the patch and Try run ? Should I ask for review ?

Comment 17

[Mozilla RelEng Bot]

Try run for beead8a9336b is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=beead8a9336b
Results (out of 176 total builds):
    success: 164
    warnings: 12
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-beead8a9336b

Comment 18

[Josh Matthews [:jdm]]

Revert the changes to nsPrivateBrowsingService.js and go for it, I say. It looks good to me!

Comment 19

[Saurabh Anand [:sawrubh]]

@Josh, Are you referring to the whitespace changes in nsPrivateBrowsingService.js. I tried removing those silly newlines but they keep coming back. Let me try again.
Should I ask you or Ehsan for review ?

Comment 20

[Josh Matthews [:jdm]]

You can cheat by making sure the patch is unapplied (if you're using MQ), then editing the patch file in .hg/patches/ and removing the entire diff for the file from the patch. For review, according to |hg log browser/modules/openLocationLastURL.jsm -f|, it looks like gavin or another Firefox peer would be a good choice.

Comment 21

[Saurabh Anand [:sawrubh]]

Hg blame showed Ehsan's name. Can I ask him for review (I'm a little scared of Gavin ;) ) ?

Comment 22

[Saurabh Anand [:sawrubh]]

Attachment: Patch v5

Fixed the nits pointed out by Josh.

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 636151 [details] [diff] [review]
Patch v5

>+                                 .QueryInterface(Ci.nsIInterfaceRequestor)
>+                                 .getInterface(Ci.nsIWebNavigation)
>+                                 .QueryInterface(Ci.nsIDocShellTreeItem)
>+                                 .rootTreeItem
>+                                 .QueryInterface(Ci.nsIInterfaceRequestor)
>+                                 .getInterface(Ci.nsIDOMWindow)
>+                                 .wrappedJSObject;

What's the point of this?

Why does OpenLocationLastURL need a window at all? Can you just pass in whether it should operate in private mode?

By the way, what's the plan for the private-browsing observer?

Comment 24

[Josh Matthews [:jdm]]

>What's the point of this?

I'm never really sure when we have a chrome browser window or not, so I've been blindly advocating for the big QI chains in general.

Personally, I prefer passing in window/loadcontext objects in general for these PB api changes rather than boolean arguments. Obviously it's the reviewer's call, however.

The private-browsing observer should observe last-pb-context-exited instead.

Comment 25

[Saurabh Anand [:sawrubh]]

@Josh, @Dao I don't get which private-browsing observer is being talked about here ? I don't see any observers that I have added or removed in my patch. Are you guys talking about this :

os.addObserver(observer, "private-browsing", true);

Comment 26

[Dão Gottwald [:dao]]

(In reply to Josh Matthews [:jdm] (travelling until June 25th) from comment #24)
> I'm never really sure when we have a chrome browser window or not, so I've
> been blindly advocating for the big QI chains in general.

Please don't do this.

(In reply to Saurabh Anand [:sawrubh] from comment #25)
> @Josh, @Dao I don't get which private-browsing observer is being talked
> about here ? I don't see any observers that I have added or removed in my
> patch. Are you guys talking about this :
> 
> os.addObserver(observer, "private-browsing", true);

Yes.

Comment 27

[(no longer active)]

(In reply to Dão Gottwald [:dao] from comment #26)
> (In reply to Josh Matthews [:jdm] (travelling until June 25th) from comment
> #24)
> > I'm never really sure when we have a chrome browser window or not, so I've
> > been blindly advocating for the big QI chains in general.
> 
> Please don't do this.

The value of the private flag on a window may change during its life-time, so we can't really get away with simply passing a boolean flag here.  However, I agree that the QI chain can be avoided here, since we control the callers and we can make sure that the thing passed in is always a chrome window (or a fake window in the case of the test, etc.)

Comment 28

[Saurabh Anand [:sawrubh]]

@Ehsan, Josh,Dao and I came to a conclusion that we could just pass the PB bool flag. Since the private flag only changes during testing, so we could just create new instances of OpenLocationLastURL every time the PB flag changes in the tests. 

What do you think ?

Comment 29

[(no longer active)]

Comment on attachment 636151 [details] [diff] [review]
Patch v5

Review of attachment 636151 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/openLocation.js
@@ +7,4 @@
>  var browser;
>  var dialog = {};
>  var pref = null;
> +let tmp = {};

Nit: please use a better name, such as openLocationModule.

@@ +15,5 @@
>    // not critical, remain silent
>  }
>  
> +Components.utils.import("resource:///modules/openLocationLastURL.jsm", tmp);
> +let gOpenLocationLastURL = new tmp.OpenLocationLastURL(window);

As I said in openLocationLastURL.jsm, this should be window.opener.

::: browser/components/privatebrowsing/test/unit/test_openLocationLastURL.js
@@ +6,5 @@
>  
>  function run_test_on_service()
>  {
> +  let tmp = {};
> +  // This singleton fakes the window required for getting the PB flag

Nit: this is technically not a singleton, just a variable.  :-)

@@ +7,5 @@
>  function run_test_on_service()
>  {
> +  let tmp = {};
> +  // This singleton fakes the window required for getting the PB flag
> +  let window = { gPrivateBrowsingUI: { privateWindow: false } };

This makes me sort of sad...  I think we should now convert this test into a browser-chrome test, so that we can test this using a real window.  If you want to do that in this bug, that's great, if not, please file a follow-up bug for that.

@@ +17,5 @@
>      Cc["@mozilla.org/observer-service;1"].
>      getService(Ci.nsIObserverService).
>      notifyObservers(null, "browser:purge-session-history", "");
>    }
> +  

Nit: please avoid adding trailing whitespaces.  :-)

::: browser/modules/openLocationLastURL.jsm
@@ +10,3 @@
>  
>  let pbSvc = Components.classes["@mozilla.org/privatebrowsing;1"]
>                        .getService(Components.interfaces.nsIPrivateBrowsingService);

You should not need to access the private browsing service directly in this file any more.  So, please remove this variable.

@@ +40,1 @@
>  os.addObserver(observer, "private-browsing", true);

As Josh mentioned, you should change this to use the new last-pb-context-exited notification, which is triggered when the last private browsing window is closed.

@@ +41,5 @@
>  os.addObserver(observer, "browser:purge-session-history", true);
>  
> +
> +function OpenLocationLastURL(aWindow) {
> +  this._window = aWindow;

Nit: Please name this this.window, no need for the underscore prefix.

@@ +55,5 @@
> +                                 .QueryInterface(Ci.nsIDocShellTreeItem)
> +                                 .rootTreeItem
> +                                 .QueryInterface(Ci.nsIInterfaceRequestor)
> +                                 .getInterface(Ci.nsIDOMWindow)
> +                                 .wrappedJSObject;

This seems sub-optimal to me.  Instead of looking at the window opener here, we should just pass the opener to OpenLocationLastURL in openLocation.js, and inside this method, we should assume that PB mode is not active if either the window parameter is null (which is the case if there is no opener) or if gPrivateBrowsingUI is not defined on it (which is the case if the opener is not a browser window).  Something like:

  // Assume not in private browsing mode, unless the browser window is
  // in private mode.
  if (!this.window || !("gPrivateBrowsingUI" in this.window))
    return false;

@@ +58,5 @@
> +                                 .getInterface(Ci.nsIDOMWindow)
> +                                 .wrappedJSObject;
> +    
> +    if (!this._window)
> +      return false;

This can be collapsed into the check I talked about above.

Comment 30

[(no longer active)]

(In reply to Saurabh Anand [:sawrubh] from comment #28)
> @Ehsan, Josh,Dao and I came to a conclusion that we could just pass the PB
> bool flag. Since the private flag only changes during testing, so we could
> just create new instances of OpenLocationLastURL every time the PB flag
> changes in the tests. 
> 
> What do you think ?

As I said above, the private flag can change during the lifetime of the window, so there is no way for us to avoid passing in the window object itself.

Comment 31

[Dão Gottwald [:dao]]

(In reply to Ehsan Akhgari [:ehsan] from comment #27)
> The value of the private flag on a window may change during its life-time,

This is irrelevant here. The flag would be passed everytime openLocation.js calls the OpenLocationLastURL constructor. It can't change while the modal dialog is open.

Comment 32

[(no longer active)]

(In reply to Dão Gottwald [:dao] from comment #31)
> (In reply to Ehsan Akhgari [:ehsan] from comment #27)
> > The value of the private flag on a window may change during its life-time,
> 
> This is irrelevant here. The flag would be passed everytime openLocation.js
> calls the OpenLocationLastURL constructor. It can't change while the modal
> dialog is open.

What makes you think that it can't?

Comment 33

[Dão Gottwald [:dao]]

It's a modal dialog... You could theoretically open that dialog, switch to a different window and enter private browsing mode, but that's kind of broken anyway.

By the way, the goal is that starting private browsing means to open a private window, isn't it? How would a window's status ever change in that world?

Comment 34

[(no longer active)]

(In reply to Dão Gottwald [:dao] from comment #33)
> It's a modal dialog... You could theoretically open that dialog, switch to a
> different window and enter private browsing mode, but that's kind of broken
> anyway.

I'm not worried about that case.  What I'm thinking about are add-ons, since they can toggle the private flag for a given window at any time.  It probably would not make sense for them to do this when a modal dialog is open, but I don't think it's a good idea to design this in a way that would break if they would.

> By the way, the goal is that starting private browsing means to open a
> private window, isn't it? How would a window's status ever change in that
> world?

It won't if there are no add-ons which set that flag (except for the corner case of when we open the private window for the first time, since we'll probably set the private flag on it some time after it's been opened, but that doesn't matter here.)

Comment 35

[Dão Gottwald [:dao]]

> It won't if there are no add-ons which set that flag

Why do we want to support that? It seems that not doing so would cut away some complexity.

Comment 36

[(no longer active)]

(In reply to Dão Gottwald [:dao] from comment #35)
> > It won't if there are no add-ons which set that flag
> 
> Why do we want to support that? It seems that not doing so would cut away
> some complexity.

I don't think that it does.  We still need to read the privateWindow flag somewhere, passing it directly to the OpenLocationLastURL or reading it off of gPrivateWindowUI in the constructor are just two different ways of doing the same thing, and the former has a trade-off which I would love to avoid if possible, especially since we implicitly support this use case today (i.e., if an add-on does something like that, this code will handle it properly.)

Comment 37

[Dão Gottwald [:dao]]

(In reply to Ehsan Akhgari [:ehsan] from comment #36)
> (In reply to Dão Gottwald [:dao] from comment #35)
> > > It won't if there are no add-ons which set that flag
> > 
> > Why do we want to support that? It seems that not doing so would cut away
> > some complexity.
> 
> I don't think that it does.  We still need to read the privateWindow flag
> somewhere, passing it directly to the OpenLocationLastURL or reading it off
> of gPrivateWindowUI in the constructor are just two different ways of doing
> the same thing,

I'm thinking more general. E.g. we'd have to close and later reopen the open tabs like we do today, something I'd hope we'd get rid of, and other stateful code would have to react to mode changes.

Comment 38

[(no longer active)]

(In reply to Dão Gottwald [:dao] from comment #37)
> (In reply to Ehsan Akhgari [:ehsan] from comment #36)
> > (In reply to Dão Gottwald [:dao] from comment #35)
> > > > It won't if there are no add-ons which set that flag
> > > 
> > > Why do we want to support that? It seems that not doing so would cut away
> > > some complexity.
> > 
> > I don't think that it does.  We still need to read the privateWindow flag
> > somewhere, passing it directly to the OpenLocationLastURL or reading it off
> > of gPrivateWindowUI in the constructor are just two different ways of doing
> > the same thing,
> 
> I'm thinking more general. E.g. we'd have to close and later reopen the open
> tabs like we do today, something I'd hope we'd get rid of,

We won't have to do that in the brave new world of per-window PB mode.  We will just open a new window, set the private flag on it, and won't touch your existing tabs and windows.

> and other stateful code would have to react to mode changes.

There are no more global notifications in the per-window API, and that is intentional.  Callers which need updated information of the state of the private flag on a loading context are expected to hold on to a docshell (or something containing one) and just read the flag directly.  This design is ultimately a lot simpler than maintaining many flags all around the code base and keeping them updated for a given docshell based on some sort of a notification about a mode change.

Comment 39

[(no longer active)]

(FWIW, https://wiki.mozilla.org/Per-window_Private_Browsing provides a basic summary of the design of the per-window PB APIs.)

Comment 40

[Dão Gottwald [:dao]]

> > I'm thinking more general. E.g. we'd have to close and later reopen the open
> > tabs like we do today, something I'd hope we'd get rid of,
> 
> We won't have to do that in the brave new world of per-window PB mode.  We
> will just open a new window, set the private flag on it, and won't touch
> your existing tabs and windows.

I was referring to the case of an add-on making the current non-ptivate window private and vice versa.

Comment 41

[(no longer active)]

(In reply to Dão Gottwald [:dao] from comment #40)
> > > I'm thinking more general. E.g. we'd have to close and later reopen the open
> > > tabs like we do today, something I'd hope we'd get rid of,
> > 
> > We won't have to do that in the brave new world of per-window PB mode.  We
> > will just open a new window, set the private flag on it, and won't touch
> > your existing tabs and windows.
> 
> I was referring to the case of an add-on making the current non-ptivate
> window private and vice versa.

No, we don't need to close and reopen the existing tabs if the add-on sets the flag.

Comment 42

[Dão Gottwald [:dao]]

Well, we're potentially leaking private data then. This doesn't seem right, nor does it seem reasonable to expect this hypothetical add-on to plug all possible data leaks in foreign code. Again, it seems to me that we'd be better off just not supporting this.

Comment 43

[(no longer active)]

(In reply to Dão Gottwald [:dao] from comment #42)
> Well, we're potentially leaking private data then. This doesn't seem right,
> nor does it seem reasonable to expect this hypothetical add-on to plug all
> possible data leaks in foreign code. Again, it seems to me that we'd be
> better off just not supporting this.

What sort of private data would we be leaking?

(We're getting off topic on this bug, even if we pass the boolean flag directly here, that still would not mean that we're not supporting this use case.  If and when we make that decision, the changes required to implement it are much more invasive than the scope of this bug.

Comment 44

[Saurabh Anand [:sawrubh]]

Attachment: Patch v6

Fixed the comments and nits pointed out by Ehsan, except the whitespace one since couldn' find it. Hope that'll be bearable ;)

Comment 45

[Saurabh Anand [:sawrubh]]

https://tbpl.mozilla.org/?tree=Try&rev=39060e04cbf9

Comment 46

[(no longer active)]

Comment on attachment 636445 [details] [diff] [review]
Patch v6

Review of attachment 636445 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below change.

::: browser/modules/openLocationLastURL.jsm
@@ +26,1 @@
>          gOpenLocationLastURLData = "";

Don't you need to modify the observer event right above this line to be last-pb-exited?

Comment 47

[Saurabh Anand [:sawrubh]]

Attachment: Patch v6

Addressed Ehsan's comments on previous patch.

Comment 48

[Saurabh Anand [:sawrubh]]

https://tbpl.mozilla.org/?tree=Try&rev=163bcb8e5c6a

Comment 49

[Saurabh Anand [:sawrubh]]

The tree looks green. If I get a r+, then I'll mark it for checkin (if Ehsan agrees ;) )

Comment 50

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1bd5333433e

Comment 51

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f1bd5333433e