832700 - Add private browsing to Firefox OS

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Browser, enhancement, P2)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Build Identifier: 20130116230203
Git commit info: 3c84fa2b74b3dbe591f0d4fbedbac9aac...

Now that private browsing mode is new and shiny in Firefox Desktop and Mobile, we should consider implementing it for Firefox OS.

People might want to buy engagement rings on a Firefox OS phone in the future. :)

Comment 1

[(no longer active)]

There will be probably some platform work involved here to make sure that the privacy status is correctly transmitted to the child process when we need it, and also a whole bunch of work to make it exposed to the mozbrowser magic.  I would definitely be interested to sit down and talk about this if people are interested in implementing this.

Comment 2

[Dietrich Ayala (:dietrich)]

Yes please, but not happening on the 1.x branch.

Comment 3

[Ben Francis [:benfrancis]]

This will probably be a meta bug and we'll need bugs filed on the platform side too, but adding to backlog for prioritisation.

Comment 5

[Matt Basta [:basta]]

As part of the browser API, a new attribute could be introduced called "nosession" or something like that. We'd obviously implement it as "moznosession".

<iframe mozbrowser moznosession ...>

Comment 6

[(no longer active)]

(In reply to comment #5)
> As part of the browser API, a new attribute could be introduced called
> "nosession" or something like that. We'd obviously implement it as
> "moznosession".
> 
> <iframe mozbrowser moznosession ...>

We cannot safely handle changing the privacy state of a docshell after it has been created.  Designing the attribute this way requires us to be able to handle the removal of the attribute in a sane way, which will be impossible.

Comment 7

[Matt Basta [:basta]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #6)
> We cannot safely handle changing the privacy state of a docshell after it
> has been created.  Designing the attribute this way requires us to be able
> to handle the removal of the attribute in a sane way, which will be
> impossible.

Rather than changing the privacy state, only allow the change to apply when the iframe is navigating or hasn't is being inserted into the DOM. So for instance:

  var ifr = document.createElement('iframe');
  ifr.mozprivate = true; // or something with setAttribute()
  ifr.src = '...';
  document.body.appendChild(ifr);

That would produce a private browsing iframe. At that point, running this code:

  ifr.mozprivate = false;

would have no effect since you can't change an active page from normal to private or vise versa. There are a few examples of changing attributes in the DOM and not having the elements behavior change.

Comment 8

[ffos-browser]

A Pivotal Tracker story has been created for this Bug: http://www.pivotaltracker.com/story/show/53548913

Comment 9

[(no longer active)]

(In reply to comment #8)
> A Pivotal Tracker story has been created for this Bug:
> http://www.pivotaltracker.com/story/show/53548913

FWIW Private Browsing is only new and shiny on Android, it was first implemented on desktop in Firefox 3.5.  :-)

Comment 10

[g]

Yeah, I just got my fxos phone this week, and I was so upset when I saw theres no private mode... Porn for example is a gigantic economy and its sad that this will work out badly on firefox os without a private mode. lol.

Any progress?

Comment 11

[[:fabrice] Fabrice Desré]

Attachment: gecko patch

This adds a mozprivatebrowsing on mozbrowser iframes that needs to be set to "true" to open a page in private browsing mode. There's no support for toggling private browsing mode (like we don't support changes to the mozapp attribute).

Comment 12

[[:fabrice] Fabrice Desré]

Attachment: gaia patch

This is terribly hackish, but that enough to test the feature. Each time the user opens a new browser tab, we ask whether it should be private or not.

We absolutely need a better UX for all that!

Comment 13

[Stephany Wilkes]

Fabrice, is this something we'd like a cleaned up version of for 2.2, or are you looking for improvement in 2.1? Thanks! This is a cool feature.

Comment 14

[[:fabrice] Fabrice Desré]

(In reply to Stephany Wilkes from comment #13)
> Fabrice, is this something we'd like a cleaned up version of for 2.2, or are
> you looking for improvement in 2.1? Thanks! This is a cool feature.

2.1 is done feature wise. I'm targeting 2.2, and I don't know how to add that nicely in our rocketbar/browser UX. Fennec has a "open new private tab" option, but it's not obvious to map that in fxOS (at least to me!).

Comment 15

[Stephany Wilkes]

Noting as a nominee for 2.2 backlog and flagging Wilfred for Sys FE consideration during 2.2 planning.

Comment 16

[[:fabrice] Fabrice Desré]

Attachment: gecko tests

Olli, I ended up removing the non-native Promise() boilerplate. This is testing private browsing in mozbrowser frames by checking that localstorage behaves as expected.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8490551 [details] [diff] [review]
gecko patch

>+  // Copy the opener frame's mozprivatebrowsing attribute to the popup frame.
>+  if (aOpenerFrameElement->HasAttr(kNameSpaceID_None, nsGkAtoms::mozprivatebrowsing)) {
>+    nsAutoString mozprivatebrowsing;
>+    aOpenerFrameElement->GetAttr(kNameSpaceID_None, nsGkAtoms::mozprivatebrowsing,
>+                                 mozprivatebrowsing);
>+    popupFrameElement->SetAttr(kNameSpaceID_None, nsGkAtoms::mozprivatebrowsing,
>+                               mozprivatebrowsing, /* aNotify = */ false);
>+  }
You don't need both HasAttr and GetAttr.
nsAutoString mozprivatebrowsing;
if (aOpenerFrameElement->GetAttr(kNameSpaceID_None, nsGkAtoms::mozprivatebrowsing,
                                 mozprivatebrowsing)) {
  popupFrameElement->SetAttr(kNameSpaceID_None, nsGkAtoms::mozprivatebrowsing,
                             mozprivatebrowsing, /* aNotify = */ false);
}
Looks like you copied the style from the code above, but there really isn't any reason for that.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8490551 [details] [diff] [review]
gecko patch

But I think ehsan should take a look at this too.

Comment 19

[(no longer active)]

Comment on attachment 8490551 [details] [diff] [review]
gecko patch

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

::: dom/browser-element/BrowserElementChild.js
@@ +56,5 @@
>                              { 'msg_name': 'hello' })[0];
>  docShell.QueryInterface(Ci.nsIDocShellTreeItem).name = infos.name;
>  docShell.setFullscreenAllowed(infos.fullscreenAllowed);
> +if (infos.isPrivate) {
> +  docShell.QueryInterface(Ci.nsILoadContext).usePrivateBrowsing = true;

I think it's probably too late to set this flag on the docshell here since we have no way of guaranteeing that nothing PB sensitive has occurred thus far.  Instead, we should this code and ensure that we create the tab child in the right mode: <http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1068>

::: dom/browser-element/BrowserElementParent.jsm
@@ +388,5 @@
>        name: this._frameElement.getAttribute('name'),
>        fullscreenAllowed:
>          this._frameElement.hasAttribute('allowfullscreen') ||
> +        this._frameElement.hasAttribute('mozallowfullscreen'),
> +      isPrivate: this._frameElement.getAttribute('mozprivatebrowsing') == 'true'

Nit: I think you want hasAttribute here.  I don't think we want to support mozprivatebrowsing=false etc.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #19)

> I think it's probably too late to set this flag on the docshell here since
> we have no way of guaranteeing that nothing PB sensitive has occurred thus
> far.  Instead, we should this code and ensure that we create the tab child
> in the right mode:
> <http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1068>
I'd be surprised if anything interesting has been loaded at that point. That would be a bug, rather major one even.
But sure, safer to set the flag as early as possible.

Comment 21

[[:fabrice] Fabrice Desré]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #19)
> Comment on attachment 8490551 [details] [diff] [review]

> I think it's probably too late to set this flag on the docshell here since
> we have no way of guaranteeing that nothing PB sensitive has occurred thus
> far.  Instead, we should this code and ensure that we create the tab child
> in the right mode:
> <http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1068>

One issue is that we pre-create tabchild to speed up overall app startup (http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#752) and at this point we don't know yet if we'll need a private browsing session.

> ::: dom/browser-element/BrowserElementParent.jsm
> @@ +388,5 @@
> >        name: this._frameElement.getAttribute('name'),
> >        fullscreenAllowed:
> >          this._frameElement.hasAttribute('allowfullscreen') ||
> > +        this._frameElement.hasAttribute('mozallowfullscreen'),
> > +      isPrivate: this._frameElement.getAttribute('mozprivatebrowsing') == 'true'
> 
> Nit: I think you want hasAttribute here.  I don't think we want to support
> mozprivatebrowsing=false etc.

Hm, if someone sets mozprivatebrowsing=false it's pretty clear they don't expect a private browsing session no? But I don't mind changing that if you prefer.

Comment 22

[(no longer active)]

(In reply to Fabrice Desré [:fabrice] from comment #21)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #19)
> > Comment on attachment 8490551 [details] [diff] [review]
> 
> > I think it's probably too late to set this flag on the docshell here since
> > we have no way of guaranteeing that nothing PB sensitive has occurred thus
> > far.  Instead, we should this code and ensure that we create the tab child
> > in the right mode:
> > <http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1068>
> 
> One issue is that we pre-create tabchild to speed up overall app startup
> (http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#752) and
> at this point we don't know yet if we'll need a private browsing session.

Can we avoid using a pre-created TabChild for the PB case?

> > ::: dom/browser-element/BrowserElementParent.jsm
> > @@ +388,5 @@
> > >        name: this._frameElement.getAttribute('name'),
> > >        fullscreenAllowed:
> > >          this._frameElement.hasAttribute('allowfullscreen') ||
> > > +        this._frameElement.hasAttribute('mozallowfullscreen'),
> > > +      isPrivate: this._frameElement.getAttribute('mozprivatebrowsing') == 'true'
> > 
> > Nit: I think you want hasAttribute here.  I don't think we want to support
> > mozprivatebrowsing=false etc.
> 
> Hm, if someone sets mozprivatebrowsing=false it's pretty clear they don't
> expect a private browsing session no? But I don't mind changing that if you
> prefer.

The issue is that we cannot properly support an iframe with mozprivatebrowsing=true having a nested iframe with mozprivatebrowsing=false with the current architecture, so I'd prefer us to not pretend so.

Comment 23

[[:fabrice] Fabrice Desré]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #22)
> (In reply to Fabrice Desré [:fabrice] from comment #21)
> > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > comment #19)
> > > Comment on attachment 8490551 [details] [diff] [review]
> > 
> > > I think it's probably too late to set this flag on the docshell here since
> > > we have no way of guaranteeing that nothing PB sensitive has occurred thus
> > > far.  Instead, we should this code and ensure that we create the tab child
> > > in the right mode:
> > > <http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1068>
> > 
> > One issue is that we pre-create tabchild to speed up overall app startup
> > (http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#752) and
> > at this point we don't know yet if we'll need a private browsing session.
> 
> Can we avoid using a pre-created TabChild for the PB case?

Well... no. Unless we can travel back in time ;)  

> The issue is that we cannot properly support an iframe with
> mozprivatebrowsing=true having a nested iframe with mozprivatebrowsing=false
> with the current architecture, so I'd prefer us to not pretend so.

ok, that makes sense then.

Comment 24

[(no longer active)]

(In reply to Fabrice Desré [:fabrice] from comment #23)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #22)
> > (In reply to Fabrice Desré [:fabrice] from comment #21)
> > > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > > comment #19)
> > > > Comment on attachment 8490551 [details] [diff] [review]
> > > 
> > > > I think it's probably too late to set this flag on the docshell here since
> > > > we have no way of guaranteeing that nothing PB sensitive has occurred thus
> > > > far.  Instead, we should this code and ensure that we create the tab child
> > > > in the right mode:
> > > > <http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1068>
> > > 
> > > One issue is that we pre-create tabchild to speed up overall app startup
> > > (http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#752) and
> > > at this point we don't know yet if we'll need a private browsing session.
> > 
> > Can we avoid using a pre-created TabChild for the PB case?
> 
> Well... no. Unless we can travel back in time ;)  

Hmm, can you please explain in more detail how this works, and what exactly happens on this pre-created TabChild?  (Pointers to the code is fine too unless it's thousands of lines of code :)

Comment 25

[[:fabrice] Fabrice Desré]

To optimize app startup time, we start a "preallocated process" when we are idle.

This creates a ContentParent/ContentChild pair, and in the child we run http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentChild.cpp#1878, calling TabChild::PreloadSlowThings() at http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#744

This one does:
- load and execute preload.js (https://mxr.mozilla.org/mozilla-central/source/dom/ipc/preload.js)
  This loads a bunch of jsm and xpcom services, and initializes the docshell to load about:blank
- initialize the presShell, to save us an initial reflow.

So at this point, the docShell has not done anything else than loading about:blank, which is still a safe point to turn on PB.

Comment 26

[(no longer active)]

(In reply to Fabrice Desré [:fabrice] from comment #25)
> To optimize app startup time, we start a "preallocated process" when we are
> idle.
> 
> This creates a ContentParent/ContentChild pair, and in the child we run
> http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentChild.cpp#1878,
> calling TabChild::PreloadSlowThings() at
> http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#744
> 
> This one does:
> - load and execute preload.js
> (https://mxr.mozilla.org/mozilla-central/source/dom/ipc/preload.js)
>   This loads a bunch of jsm and xpcom services, and initializes the docshell
> to load about:blank
> - initialize the presShell, to save us an initial reflow.
> 
> So at this point, the docShell has not done anything else than loading
> about:blank, which is still a safe point to turn on PB.

OK, agreed, that would make sense.  However I would like us to add some debugging code to verify the last sentence on the off chance that this setup changes in the future without us remembering to take PB into account.

Comment 27

[[:fabrice] Fabrice Desré]

Attachment: gecko patch v2

Ehsan, I added the check we talked about. Tests are still passing!

Comment 28

[[:fabrice] Fabrice Desré]

Attachment: gecko patch v2.1

Comment 29

[(no longer active)]

Comment on attachment 8503420 [details] [diff] [review]
gecko patch v2.1

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

Looks great!

::: docshell/base/nsDocShell.cpp
@@ +2285,5 @@
>  NS_IMETHODIMP
> +nsDocShell::GetHasLoadedNonBlankURI(bool* aResult)
> +{
> +    NS_ENSURE_ARG_POINTER(aResult);
> +    

Nit: trailing whitespace.

Comment 30

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/660b7e69fac7
https://hg.mozilla.org/integration/b2g-inbound/rev/9d6312427933

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Both backed out in https://hg.mozilla.org/integration/b2g-inbound/rev/acc80d67a89f for mochitest-2 bustage: 
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=624365&repo=b2g-inbound

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This also seems to have broken an android test: https://treeherder.mozilla.org/ui/logviewer.html#?job_id=624784&repo=b2g-inbound

Comment 33

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/3efe998b6136
https://hg.mozilla.org/integration/b2g-inbound/rev/069cb3dad411

Comment 34

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Stephany Wilkes from comment #15)
> Noting as a nominee for 2.2 backlog and flagging Wilfred for Sys FE
> consideration during 2.2 planning.

Please file a follow-up bug for Gaia part in FxOS: Gaia::System::Window mgmt.

Comment 35

[[:fabrice] Fabrice Desré]

That was backed out in https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=3e18046306fc for M2 failures due to the oop and inproc tests running from the same profile which caused the localstorage to not be cleared properly.

Fixed in try build https://tbpl.mozilla.org/?tree=Try&rev=d891dbd3e391

Comment 36

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/0990e633fd0d
https://hg.mozilla.org/integration/b2g-inbound/rev/89b935e3119d

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0990e633fd0d
https://hg.mozilla.org/mozilla-central/rev/89b935e3119d

Comment 38

[[:fabrice] Fabrice Desré]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #34)
> (In reply to Stephany Wilkes from comment #15)
> > Noting as a nominee for 2.2 backlog and flagging Wilfred for Sys FE
> > consideration during 2.2 planning.
> 
> Please file a follow-up bug for Gaia part in FxOS: Gaia::System::Window mgmt.

It's bug 1081731