Closed Bug 832700 Opened 11 years ago Closed 10 years ago

Add private browsing to Firefox OS

Categories

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

All
Gonk (Firefox OS)
enhancement

Tracking

(feature-b2g:2.2?, b2g18-)

RESOLVED FIXED
2.1 S7 (24Oct)
feature-b2g 2.2?
Tracking Status
b2g18 - ---

People

(Reporter: gkw, Assigned: fabrice)

References

Details

(4 keywords, Whiteboard: c=browser u=user)

Attachments

(3 files, 2 obsolete files)

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. :)
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.
Yes please, but not happening on the 1.x branch.
Keywords: feature
This will probably be a meta bug and we'll need bugs filed on the platform side too, but adding to backlog for prioritisation.
Whiteboard: c=browser u=user
Priority: -- → P2
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 ...>
(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.
(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.
Flags: needinfo?(ibarlow)
Keywords: uiwanted
A Pivotal Tracker story has been created for this Bug: http://www.pivotaltracker.com/story/show/53548913
(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.  :-)
Flags: needinfo?(ibarlow)
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?
Attached patch gecko patch (obsolete) — Splinter Review
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).
Assignee: nobody → fabrice
Attached patch gaia patchSplinter Review
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!
Flags: needinfo?(firefoxos-ux-bugzilla)
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.
(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!).
Noting as a nominee for 2.2 backlog and flagging Wilfred for Sys FE consideration during 2.2 planning.
feature-b2g: --- → 2.2?
Flags: needinfo?(firefoxos-ux-bugzilla) → needinfo?(wmathanaraj)
Attached patch gecko testsSplinter Review
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.
Attachment #8499910 - Flags: review?(bugs)
Attachment #8490551 - Flags: review?(bugs)
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.
Attachment #8490551 - Flags: review?(bugs) → review+
Comment on attachment 8490551 [details] [diff] [review]
gecko patch

But I think ehsan should take a look at this too.
Attachment #8490551 - Flags: review?(ehsan.akhgari)
Attachment #8499910 - Flags: review?(bugs) → review+
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.
Attachment #8490551 - Flags: review?(ehsan.akhgari) → review-
(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.
(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.
(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.
(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.
(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 :)
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.
(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.
Attached patch gecko patch v2 (obsolete) — Splinter Review
Ehsan, I added the check we talked about. Tests are still passing!
Attachment #8490551 - Attachment is obsolete: true
Attachment #8503255 - Flags: review?(ehsan.akhgari)
Attached patch gecko patch v2.1Splinter Review
Attachment #8503255 - Attachment is obsolete: true
Attachment #8503255 - Flags: review?(ehsan.akhgari)
Attachment #8503420 - Flags: review?(ehsan.akhgari)
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.
Attachment #8503420 - Flags: review?(ehsan.akhgari) → review+
(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.
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
https://hg.mozilla.org/mozilla-central/rev/0990e633fd0d
https://hg.mozilla.org/mozilla-central/rev/89b935e3119d
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S7 (Oct24)
Blocks: 1081731
(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
Depends on: 1097627
Flags: needinfo?(wmathanaraj)
See Also: → 1149892
You need to log in before you can comment on or make changes to this bug.