Add private browsing to Firefox OS

RESOLVED FIXED in 2.1 S7 (24Oct)

Status

Firefox OS
Gaia::Browser
P2
enhancement
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: gkw, Assigned: fabrice)

Tracking

(4 keywords)

unspecified
2.1 S7 (24Oct)
All
Gonk (Firefox OS)
b2g-testdriver, feature, uiwanted, unagi
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(feature-b2g:2.2?, b2g18-)

Details

(Whiteboard: c=browser u=user)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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

5 years ago
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.
tracking-b2g18: ? → -
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

Updated

5 years ago
Duplicate of this bug: 879248

Comment 5

5 years ago
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

5 years ago
(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

5 years ago
(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

Comment 8

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

Comment 9

5 years ago
(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.  :-)

Updated

4 years ago
Flags: needinfo?(ibarlow)

Comment 10

4 years ago
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?
(Assignee)

Comment 11

4 years ago
Created attachment 8490551 [details] [diff] [review]
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).
Assignee: nobody → fabrice
(Assignee)

Comment 12

4 years ago
Created attachment 8490552 [details] [diff] [review]
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!
(Assignee)

Updated

4 years ago
Flags: needinfo?(firefoxos-ux-bugzilla)

Comment 13

4 years ago
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.
(Assignee)

Comment 14

4 years ago
(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

3 years ago
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)
(Assignee)

Comment 16

3 years ago
Created attachment 8499910 [details] [diff] [review]
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.
Attachment #8499910 - Flags: review?(bugs)
(Assignee)

Updated

3 years ago
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)

Updated

3 years ago
Attachment #8499910 - Flags: review?(bugs) → review+

Comment 19

3 years ago
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.
(Assignee)

Comment 21

3 years ago
(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

3 years ago
(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.
(Assignee)

Comment 23

3 years ago
(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

3 years ago
(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 :)
(Assignee)

Comment 25

3 years ago
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

3 years ago
(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.
(Assignee)

Comment 27

3 years ago
Created attachment 8503255 [details] [diff] [review]
gecko patch v2

Ehsan, I added the check we talked about. Tests are still passing!
Attachment #8490551 - Attachment is obsolete: true
Attachment #8503255 - Flags: review?(ehsan.akhgari)
(Assignee)

Comment 28

3 years ago
Created attachment 8503420 [details] [diff] [review]
gecko patch v2.1
Attachment #8503255 - Attachment is obsolete: true
Attachment #8503255 - Flags: review?(ehsan.akhgari)
Attachment #8503420 - Flags: review?(ehsan.akhgari)

Comment 29

3 years ago
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.
(Assignee)

Comment 35

3 years ago
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
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S7 (Oct24)
(Assignee)

Updated

3 years ago
Blocks: 1081731
(Assignee)

Comment 38

3 years ago
(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)

Updated

3 years ago
See Also: → bug 1149892
You need to log in before you can comment on or make changes to this bug.