248970 - (PrivateBrowsing) Private Browsing mode (global toggle for saving/caching everything)

Status: VERIFIED FIXED

(Firefox :: Private Browsing, enhancement)

Description

[Matthias Himber]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040615 Firefox/0.9
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040615 Firefox/0.9

In the next version of OS X, Safari will have a "Private Browsing" mode which,
basically, disables writing to cache, URL history, etc.
I think this is a good idea, and it's small and useful enough that it should go
into the browser, not an extension.

Reproducible: Always
Steps to Reproduce:

Comment 1

[Mike Connor [:mconnor]]

So basically, implement some sort of universal pref to toggle all of the various
cache/cookies/etc off without opening prefs?  I don't know if this deserves a
toggle in this manner, or how practical that is.

morphing summary to describe what this actually does in Safari (whenever Tiger
comes out)

The flipside to this is the plan to have Ctrl-Shift-C act as a global privacy
clear option, so that if you want to clear a certain group of things, you can
set it to do that.

Comment 2

[Matthias Himber]

(In reply to comment #1)
No, more like a "don't remember what I'm doing now" toggle, which would allow
browsing sites without cookies/history/cache being written to. This would allow
users to do any privacy-sensitive browsing (online banking on a public computer,
shopping for gifts on the family computer, etc.) without having to completely
clear history/cache/cookies afterwards. This is more useful because when
clearing all privacy sensitive material you would loose for example login
cookies for other sites, the complete history which may contain useful items,
and all of the cache making browsing slower until the cache is refilled.
Privacy mode is also more complicated than a global privacy clear option, as it
means cookies and history would have to be moved to a two-layer memory/disk
model like cache, and individual items would need to have a flag that prevents
them to move from memory to disk. This is necessary beause the browser would
still need to handle cookies and history, just not write them to disk
completely. I'm not sure wether this can be done in an extension at all.

Comment 3

[Jesse Ruderman]

See also bug 155300 (guest profiles) and bug 103765 (anonymous mode).

Comment 4

[Simon Paquet [:sipaq]]

*** Bug 260266 has been marked as a duplicate of this bug. ***

Comment 5

[u49640]

would be a great addition for Firefox, esp. if you have to use a public Terminal

(and it would be great for the Mac 1.0 to have this feature before safari gets it)

btw: shouldnt this be moved to browser, since the suite could use this too?

Comment 6

[Mike Connor [:mconnor]]

If Browser needs a bug for it too, great, file a bug there.  No one still
working/triaging Seamonkey really cares about Firefox.

Comment 7

[Sylvain Pasche]

See http://live.gnome.org/Epiphany_2fFeatureDesign_2fPrivateBrowsing for a
discussion related to this.

Comment 8

[R Vietri]

It is an extremely useful feature specifically for internet cafes or machines
which are used by multiple users with the same login or even for the security
paranoid. It is not that difficult to implement either. Definately something I
would be keen to see and would be one less feature safari has that we don't.

Comment 9

[Kelley Cook]

FWIW, this feature has been seperately implemented by both the Stealther and Distrust extensions.

Which means someone can probably close this bug.

Comment 10

[Matthias Himber]

(In reply to comment #9)
> Which means someone can probably close this bug.
second.

Comment 11

[José Jeria]

(In reply to comment #9)
> FWIW, this feature has been seperately implemented by both the Stealther and
> Distrust extensions.
> 
> Which means someone can probably close this bug.

Since when should bugs be closed because there is an extension for it?

Comment 12

[Michael Ventnor]

I'll take this. I managed to get a working mode today (purely back-end and pref based, no UI just yet), and I'll post the patch in the morning once I find suitable reviewers.

Comment 13

[Michael Ventnor]

Attachment: First try

If Private Browsing ("privacy.privatebrowsing") is turned on:

* Cache won't be touched.
* Nothing will be added to Places history.
* No new entries will be saved in autocomplete.
* Download records will delete themselves when the download is complete.
* Cookies are kept until Private Browsing is turned off, or the browser closes, at which point the cookies will be deleted.

I can almost guarantee this won't break anything, because all this does is it sneakily manipulates pref-check functions and return values in each section to do the right thing.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> I can almost guarantee this won't break anything

view-source and meta charset reloads come to mind as things it could break.  Also wyciwyg:.  I assume you've tested all of these?  I assume you've also tested the performance impact of getting the pref every single time and determined that it's cheap enough that caching it is not worth it?

This probably also breaks visited link coloring, unless I'm missing something.

In any case, I'm not going to be able to review this thoroughly in a reasonable timeframe.  Please ask someone else?  biesi might be a good idea.

Comment 15

[Michael Ventnor]

Comment on attachment 267216 [details] [diff] [review]
First try

(In reply to comment #14)
> view-source and meta charset reloads come to mind as things it could break.

view-source works.

> Also wyciwyg:.  I assume you've tested all of these? 

I haven't yet tested meta charset reloads or wyciwyg, I'm not really sure how to test them. The reason for my (over)confidence is that this patch only really changes existing preference or policy checks.

> I assume you've also
> tested the performance impact of getting the pref every single time and
> determined that it's cheap enough that caching it is not worth it?

I can't find an impact at all. Some of the functions which I change already seem to check a pref every time they're called.

> This probably also breaks visited link coloring, unless I'm missing something.

No, this does as expected (expected IMO being: visited link colours show up when a link is visited outside of Private Browsing, but not when its visited while turned on).

> In any case, I'm not going to be able to review this thoroughly in a reasonable
> timeframe.  Please ask someone else?  biesi might be a good idea.
> 

OK.

Comment 16

[Michael Ventnor]

(In reply to comment #15)
> I can't find an impact at all. Some of the functions which I change already
> seem to check a pref every time they're called.

I actually forgot to mention the primary reason, many existing pref listeners in each section of Mozilla that this patch touches have a branch (eg "browser.foo") and changing the existing nsIPrefBranch2, or implementing a separate one, for each section would be messier than just reading the pref when needed.

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 267216 [details] [diff] [review]
First try

I'm not the right person to review this, and I don't think biesi is the right sr -- you really want mconnor, at the very least.

Comment 18

[Michael Ventnor]

Comment on attachment 267216 [details] [diff] [review]
First try

(In reply to comment #17)
> (From update of attachment 267216 [details] [diff] [review])
> I'm not the right person to review this, and I don't think biesi is the right
> sr -- you really want mconnor, at the very least.
> 

Thats precisely the problem I had before I submitted a patch like this, which touched different sections of Mozilla. From the list of reviewers at http://www.mozilla.org/hacking/reviewers.html you were under toolkit and biesi was under netwerk. 

I'll get mconnor.

Comment 19

[Shawn Wilsher :sdwilsh]

Comment on attachment 267216 [details] [diff] [review]
First try


> PRInt32 
> nsDownloadManager::GetRetentionBehavior()
> {
>   // We use 0 as the default, which is "remove when done"
>   nsresult rv;
>   nsCOMPtr<nsIPrefBranch> pref = do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
>   NS_ENSURE_SUCCESS(rv, 0);
>+
>+  // The user is in private browsing mode, delete the download record when
>+  // completed
>+  PRBool privMode = PR_FALSE;
>+  pref->GetBoolPref("privacy.privatebrowsing", &privMode);
>+  if (privMode)
>+    return 0;
Please declare a constant at the top of the file, just as everything else that uses a pref does in the file.

Also, this will require some UI changes if the download manager is already open:
http://mxr.mozilla.org/seamonkey/source/toolkit/mozapps/downloads/content/downloads.js#152

Comment 21

[Mike Connor [:mconnor]]

So, I thought I'd commented here, but I didn't.  I think this is a good start, but there's some things I think need addressing:

* Toggling the mode on/off

Using prefs and/or pref observers seems kinda wrong, why not just have observers/notifications?

* Cookies

This is a pretty painful impl, since this would mean that you'll lose cookies that you had before you started in private browsing mode and accessed while you're there.  Fortunately, the cookie service now has a creationTime that would be useful here.  I think only clearing new cookies is acceptable for this feature, since stuff you've already been to will likely be in your history as well...

* History

It makes more sense to kill everything from the visits table, and remove any URLs that have zero visits.  Achieves the same thing, but means link coloring etc still work during the session.  I'm not sure if we can suppress flushing to disk, but if not I'm not as concerned about "must never touch magnetic media" as some are.

* Cache

Totally disabling cache feels like a mistake.  Why not just disable the disk cache, and clear memory cache before re-enabling disk cache?  (biesi, does disabling the disk cache preclude reloading from disk on exit of private browsing?)

* Downlaods

I bet sdwilsh's comments are bang on, so I'll stay quiet.

Comment 22

[Sy Ali]

A non-programmer suggestion:

What would happen if a 'guest' temporary user were whipped up and the browser were restarted using that profile?  Would this sort of feature be more thorough or be a more "clean" way to implement a feature like this?

A restart isn't not as convenient as having a button/hotkey to enable/disable private browsing, but it might be something to think about.

Comment 23

[Håkan Waara]

(In reply to comment #22)
> A non-programmer suggestion:
> 
> What would happen if a 'guest' temporary user were whipped up and the browser
> were restarted using that profile?  Would this sort of feature be more thorough
> or be a more "clean" way to implement a feature like this?

That would not be this feature (see comment 0), since you wouldn't be able to access to any of your current bookmarks or history.

Note that this feature already exists in Safari, and it looks like we're going to try to implement it more or less the same way.

Comment 24

[Michael Ventnor]

(In reply to comment #21)
> * Toggling the mode on/off
> 
> Using prefs and/or pref observers seems kinda wrong, why not just have
> observers/notifications?

It'd be more code, but I suppose that's best.

> * Cookies
> 
> This is a pretty painful impl, since this would mean that you'll lose cookies
> that you had before you started in private browsing mode and accessed while
> you're there.  Fortunately, the cookie service now has a creationTime that
> would be useful here.  I think only clearing new cookies is acceptable for this
> feature, since stuff you've already been to will likely be in your history as
> well...

I wrote this patch before the cookie rewrite ;) I wanted to do that but couldn't before because creation "time" was just an incremented counter. I can do that now that the DB uses a proper creationTime.

> * History
> 
> It makes more sense to kill everything from the visits table, and remove any
> URLs that have zero visits.  Achieves the same thing, but means link coloring
> etc still work during the session.  I'm not sure if we can suppress flushing to
> disk, but if not I'm not as concerned about "must never touch magnetic media"
> as some are.

Put on a tinfoil hat for a sec. Do we want link colouring to work? It can be used as another method to trace your steps...

> * Cache
> 
> Totally disabling cache feels like a mistake.  Why not just disable the disk
> cache, and clear memory cache before re-enabling disk cache?  (biesi, does
> disabling the disk cache preclude reloading from disk on exit of private
> browsing?)

I don't know much about how cache works, that just seemed like an easy way to go.

> * Downlaods
> 
> I bet sdwilsh's comments are bang on, so I'll stay quiet.

Like I said, I should probably know more about the code I'm touching before I touch it ;)
That being said, this isn't a top priority for me right now as I have other bugs I really want fixed. I wouldn't object if anyone else wanted to take this in the meantime.

Comment 25

[Jesse Ruderman]

> Put on a tinfoil hat for a sec. Do we want link colouring to work? It can be
> used as another method to trace your steps...

I think the idea is to make link coloring work, but just during the private browsing session.  After the session, that (temporary) history data goes away, and the links are no longer marked as visited.

Comment 26

[Michael Ventnor]

(In reply to comment #25)
> > Put on a tinfoil hat for a sec. Do we want link colouring to work? It can be
> > used as another method to trace your steps...
> 
> I think the idea is to make link coloring work, but just during the private
> browsing session.  After the session, that (temporary) history data goes away,
> and the links are no longer marked as visited.
> 

I know, but I was wondering if we even want that....

Comment 27

[Alex Faaborg [:faaborg] (Firefox UX)]

Michael, thanks for working on this bug, this is a feature I would personally very much like to see land in Firefox 3.

I have a few questions about the implementation that might impact the UI:

1) Are we still considering launching a new instance, which would potentially enable us to display Firefox with a modified theme?
http://wiki.mozilla.org/PrivateBrowsing

2) If not, would it be possible to modify the color and opacity of the current theme, similar to how Personas for Firefox doesn't require a restart?
http://www.puffinlabs.com/personas/

There hasn't been much activity on this bug over the last month, can anyone give me an update on what the status is?

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #27)
> 1) Are we still considering launching a new instance, which would potentially
> enable us to display Firefox with a modified theme?
> http://wiki.mozilla.org/PrivateBrowsing
> 
> 2) If not, would it be possible to modify the color and opacity of the current
> theme, similar to how Personas for Firefox doesn't require a restart?
> http://www.puffinlabs.com/personas/

Alex, Madhava, Connor and I discussed this in various fora, and I think we've actually agreed that heavy modifications to the theme on entry of Private Browsing mode would probably be antithetical to its purpose, as it would call attention to the very fact that the user had entered a mode all about being unobserved.

Comment 29

[(no longer active)]

I'm going to give this one a serious shot...  :-)

Comment 30

[(no longer active)]

(In reply to comment #19)
> Also, this will require some UI changes if the download manager is already
> open:
> http://mxr.mozilla.org/seamonkey/source/toolkit/mozapps/downloads/content/downloads.js#152
> 

Shawn: I'm afraid that the link above may not point to what you intended to say any more.  Would you mind re-stating your concern in comment 19 so that I can make sure to consider it?

Thanks!

Comment 31

[[e-mail]]

(In reply to comment #30)
> Shawn: I'm afraid that the link above may not point to what you intended to say
> any more.

fyi, you can use bonsai to figure it out - load up the cvs log for the file:
http://bonsai.mozilla.org/cvslog.cgi?file=/mozilla/toolkit/mozapps/downloads/content/downloads.js&rev=HEAD&mark=1.130
find the appropriate revision based on date, click on the linkified revision number, click on 'blame' at the bottom of the diff screen, then find the appropriate line number.

for this reason, it's always better to provide bonsai links to a particular revision & line number - that way, they don't change with time. ;)

Comment 32

[(no longer active)]

(In reply to comment #21)
> So, I thought I'd commented here, but I didn't.  I think this is a good start,
> but there's some things I think need addressing:
> 
> * Toggling the mode on/off
> 
> Using prefs and/or pref observers seems kinda wrong, why not just have
> observers/notifications?

Hmm, using prefs, users can switch to private browsing mode, close the browser,
and open it and continue in the private browsing mode...  Unless of course we
don't want that; but not saving this value for next startups seems inconsistent
with what we do in other parts of the UI.

Example: we put a check mark besides View > Status Bar to indicate that the
status bar is shown.  We save this value for next startups.  If we put a check
mark next to the "Private Browsing Mode" menu item, the users would expect the
"check mark" to be preserved in the next startup, and may assume that it is and
gets bitten later on when she discovers that this value has not been saved.

Or am I totally missing something here?

> * Cookies
> 
> This is a pretty painful impl, since this would mean that you'll lose cookies
> that you had before you started in private browsing mode and accessed while
> you're there.  Fortunately, the cookie service now has a creationTime that
> would be useful here.  I think only clearing new cookies is acceptable for this
> feature, since stuff you've already been to will likely be in your history as
> well...

I should delve into some code to get how I should use the creationTime value...

> * History
> 
> It makes more sense to kill everything from the visits table, and remove any
> URLs that have zero visits.  Achieves the same thing, but means link coloring
> etc still work during the session.  I'm not sure if we can suppress flushing to
> disk, but if not I'm not as concerned about "must never touch magnetic media"
> as some are.

Two concerns:

1. The goal here is to respect user's privacy.  Suppose that the user enters
the private browsing mode, and performs a Google search and navigates to one of
the results in a new tab.  Now, if someone sees the Google results page over
the user's shoulders, they would immediately figure out what page the user has
visited.  If I want to get over-paranoid, I could even imagine them to memorize
the page title and then do a Google search of their own to find out exactly
what the user has viewed in their private browsing mode...

2. A more serious issue: if we write the URLs to the Places DB, with the
intention of clearing them off later, and in between we crash or get closed, on
the next startup, the URLs visited in the *private* mode exist in the DB (and
we guarantee that the DB survives our crash...).  Even if we add code to handle
that in the startup, what happens if someone takes the raw sqlite DB file from
the user's profile, and open it in their own app/viewer?

The paragraph I quote below from <http://wiki.mozilla.org/PrivateBrowsing>
seems to be very relevant here:

"By choosing to write *some* data to disk (perhaps in an encrypted format) we
have broken a clear and easy to understand contract between Firefox and the
user. The user / security expert will not be sure that there is no security
risk."

We don't want that, I guess.  Not getting visited link colors is something that
the ordinary user may not even notice (there are a lot of sites which disable
our default coloring via CSS) and the advanced user will appreciate (see point
1 above).  And we can document that this is intentional in the user docs for
the private browsing feature...

> * Cache
> 
> Totally disabling cache feels like a mistake.  Why not just disable the disk
> cache, and clear memory cache before re-enabling disk cache?  (biesi, does
> disabling the disk cache preclude reloading from disk on exit of private
> browsing?)

I guess this is the right approach here...

Comment 33

[(no longer active)]

(In reply to comment #31)
> (In reply to comment #30)
> > Shawn: I'm afraid that the link above may not point to what you intended to say
> > any more.
> 
> fyi, you can use bonsai to figure it out - load up the cvs log for the file:
> http://bonsai.mozilla.org/cvslog.cgi?file=/mozilla/toolkit/mozapps/downloads/content/downloads.js&rev=HEAD&mark=1.130
> find the appropriate revision based on date, click on the linkified revision
> number, click on 'blame' at the bottom of the diff screen, then find the
> appropriate line number.

Thanks for the tip!

> for this reason, it's always better to provide bonsai links to a particular
> revision & line number - that way, they don't change with time. ;)

Agreed!  For future reference, here's the bonsai link to the code Shawn was talking about: <http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/mozapps/downloads/content/downloads.js&rev=1.60&root=/cvsroot#152>

Comment 34

[(no longer active)]

(In reply to comment #28)
> (In reply to comment #27)
> > 1) Are we still considering launching a new instance, which would potentially
> > enable us to display Firefox with a modified theme?
> > http://wiki.mozilla.org/PrivateBrowsing
> > 
> > 2) If not, would it be possible to modify the color and opacity of the current
> > theme, similar to how Personas for Firefox doesn't require a restart?
> > http://www.puffinlabs.com/personas/
> 
> Alex, Madhava, Connor and I discussed this in various fora, and I think we've
> actually agreed that heavy modifications to the theme on entry of Private
> Browsing mode would probably be antithetical to its purpose, as it would call
> attention to the very fact that the user had entered a mode all about being
> unobserved.

I agree.  In fact, I think that having a small icon in the status bar would be sufficient (if we even decide to have *that*), plus a notification (as mentioned in <http://wiki.mozilla.org/PrivateBrowsing#Making_Sure_the_User_has_the_Correct_Mental_Model> on the point when the user enters (or leaves) the private browsing mode.

Comment 35

[(no longer active)]

Attachment: WIP Patch 1 - Cache Completed

Firstly, thanks to Michael Ventnor for his work on this bug which allowed me to take over and build upon it.

Here is the first version of my WIP.  This is an unbitrotted version of Michael's patch.  I have completed the implementation regarding disk cache in this patch according to mconnor's comments.

This patch disables the disk and offline cache devices when the privacy.privatebrowsing pref is set to true, and re-enables them when it's set to false.  Upon the change of this pref's value to false, the code empties the memory cache, so that the items entered into the cache during user's private browsing period are no longer accessible after exiting this mode.  No data is written to disk when the user is in private browsing mode.

Comment 36

[(no longer active)]

(In reply to comment #19)
> Also, this will require some UI changes if the download manager is already
> open:
> http://mxr.mozilla.org/seamonkey/source/toolkit/mozapps/downloads/content/downloads.js#152

Bug 394039 has reworked the download manager UI to get notified when a download is removed, so downloads.js no longer touches the browser.download.manager.retention pref, and the change you mentioned is no longer necessary.

(See the check-in for bug 394039: <http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=downloads.js&branch=&root=/cvsroot&subdir=/mozilla/toolkit/mozapps/downloads/content&command=DIFF_FRAMESET&rev1=1.92&rev2=1.93>)

Comment 37

[(no longer active)]

Attachment: WIP Patch 2 - nsNavHistory Optimizations

Since nsNavHistory::CanAddURI() is executed commonly, and since we expect privacy.privatebrowsing to change only occasionally, it makes sense to cache the value of this pref, thus speeding up nsNavHistory::CanAddURI().  This should address part of the concerns in comment 14.

Comment 38

[Alex Faaborg [:faaborg] (Firefox UX)]

I'm really happy to see some activity in this bug.  Should we spin off a separate bug for designing the user interface for entering and exiting private browsing mode?

Comment 39

[(no longer active)]

Yes.  I'm willing to work on that as well.  I'll file a bug and add the dependency here.

In the meamntime, I'd like to have your opinion on comment 34.  Thanks!

Comment 40

[(no longer active)]

BTW, I'll do my best to land both the back-end and the UI for Beta 3, if I can get timely reviews.  I think it's important that this gets tested by users in Beta 3 and we get some user feedback about it.

Comment 41

[Alex Faaborg [:faaborg] (Firefox UX)]

The last time the ux team talked about private browsing we agreed that a small indicator to the left of the site button was the best way to go.  Let me check with Beltzner to see if I can give this some cycles, and I'll post some mockups. 

Comment 42

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #34)
> I agree.  In fact, I think that having a small icon in the status bar would be
> sufficient (if we even decide to have *that*), plus a notification (as
> mentioned in http://wiki.mozilla.org/PrivateBrowsing#Making_Sure_the_User_has_the_Correct_Mental_Model
>on the point when the user enters (or leaves) the private browsing mode.

Yup, I think that's right. The idea that Alex is referring to in comment #41 was that upon entering the mode a small indicator would appear somewhere near (or in?) the location bar that would also act as the "exit private browsing mode" button. Having it only in the status bar might not provide a good enough affordance for ending the "private mode" transaction.

Comment 43

[(no longer active)]

I filed bug 411929 to track the progress on the private browsing UI.  Some mockups would be great, Alex.  I agree that something near the location bar could be better noticed by the users.

BTW, Beltzner, did you intentionally change the Target Milestone field to Future?

Comment 44

[(no longer active)]

Attachment: WIP Patch 3 - nsFormHistory Optimizations

This patch includes the same optimization (comment 37) to nsFormHistory-related part.

Comment 45

[[e-mail]]

(In reply to comment #24)
> > * Cookies
> > 
> > This is a pretty painful impl, since this would mean that you'll lose cookies
> > that you had before you started in private browsing mode and accessed while
> > you're there.  Fortunately, the cookie service now has a creationTime that
> > would be useful here.  I think only clearing new cookies is acceptable for this
> > feature, since stuff you've already been to will likely be in your history as
> > well...
> 
> I wrote this patch before the cookie rewrite ;) I wanted to do that but
> couldn't before because creation "time" was just an incremented counter. I can
> do that now that the DB uses a proper creationTime.

yep, this sounds good to me - note that we don't expose that information via the cookie interface yet, it'd live here:
http://mxr.mozilla.org/mozilla/source/netwerk/cookie/public/nsICookie2.idl

but file a bug against me if you want it, and i'll whip up a patch.

Comment 46

[(no longer active)]

(In reply to comment #45)
> yep, this sounds good to me - note that we don't expose that information via
> the cookie interface yet, it'd live here:
> http://mxr.mozilla.org/mozilla/source/netwerk/cookie/public/nsICookie2.idl
> 
> but file a bug against me if you want it, and i'll whip up a patch.

Done: bug 411952.  Thanks!

I'll modify the cookies related part of this patch as soon as you post a patch on that bug.  The updated IDL would suffice for me to get started.  :-)

Comment 47

[(no longer active)]

SessionStoreService must also be modified to make sure the data for windows/tabs are not saved during the private browsing mode.  A patch containing this change will be posted shortly.

Comment 48

[Simon Bünzli]

(In reply to comment #47)
Yeah, you'll probably want to make SessionStore behave as when .resume_from_crash is disabled (nothing will be written to disk) and at the end clean up similarly to what we do for "browser:purge-session-history"... and just ask me for review when you're ready.

Comment 49

[(no longer active)]

Attachment: WIP Patch 4 - SessionStore

(In reply to comment #48)
> (In reply to comment #47)
> Yeah, you'll probably want to make SessionStore behave as when
> .resume_from_crash is disabled (nothing will be written to disk) and at the end
> clean up similarly to what we do for "browser:purge-session-history"... and
> just ask me for review when you're ready.

This patch changes the SessionStore service to support privacy.privatebrowsing pref, as suggested above by Simon.

The user experience is summarized as below:

When the user enters private browsing mode, the entire session is considered private, so the SessionStore service gets is prevented from writing anything to the disk, and the existing sessionstore.js file is deleted from the user's profile.  The only services from the SessionStore available during this period are the window/tab undo features.  The SessionStore continues to collect information about open tabs/windows in the memory.

When the user leaves the private browsing mode, the entire data collected in the memory would be deleted (so that after leaving the private browsing mode, the closed windows/tabs from the sessions cannot be restored.  Then, the data for current windows/tabs are collected from scratch, and saved (if necessary) to disk.  This way, nothing done inside the private browsing mode can be reflected by the SessionStore service after leaving this mode.  The reason that the data for current windows/tabs are recollected is that the user should not consider any tabs/windows she leaves open before leaving the private browsing mode private.

Comment 50

[(no longer active)]

The Password Manager should not prompt the user to save new passwords or change already saved passwords in the private browsing mode.  I'll post a new WIP patch with this change shortly.

Comment 51

[u49640]

Should the "Private Browsing" mode use the normal cookies?

Reason for my question is: What exactly is this private browsing mode?

Just dont keep any local tracks during this session?
Or dont use my "Identity" (aka cookies) for the webpages during the session?
Or Both?

Comment 52

[(no longer active)]

(In reply to comment #51)
> Should the "Private Browsing" mode use the normal cookies?

Yes.

> Reason for my question is: What exactly is this private browsing mode?

See <http://wiki.mozilla.org/PrivateBrowsing>.

Quote:
"The purpose of private browsing is to put Firefox into a temporary state where no information about the user's browsing session is stored locally."

Based on the above definition:

> Just dont keep any local tracks during this session?
> Or dont use my "Identity" (aka cookies) for the webpages during the session?
> Or Both?

The former is what Private Browsing is about.

Comment 53

[(no longer active)]

Attachment: WIP Patch 5 - nsLoginManagerPrompter

This patch implements the changes suggested in comment 50.

The user experience is as follows:

While the user is in the private browsing mode, the password manager continues to fill in the usernames and passwords already stored in the user's profile, but does not prompt the user to save new authentication information in any manner.  After the user exits from the private browsing mode, the password manager will prompt her to store any new authentications as needed, just like before she entered the private browsing mode.

Comment 54

[Alex Faaborg [:faaborg] (Firefox UX)]

>> Just dont keep any local tracks during this session?
>> Or dont use my "Identity" (aka cookies) for the webpages during the session?
>> Or Both?
>
>The former is what Private Browsing is about.

That's correct based on how we spec'd this out on the wiki page, but are we sure that this is the right behavior?  Here are two examples:

-Alex is shopping for an engagement ring, so he enters into private browsing mode and loads amazon.com.  Amazon recognizes Alex based on the cookies stored in his profile, and remembers that he spent the afternoon looking for engagement rings.  The next day Alex's soon to be fiancee sits down at his computer and loads up amazon.com (while not in private browsing mode) and sees a homepage full of "Other people who shopped for engagement rings looked at these rings as well! and Here is your recent shopping history!"

-Anna is trying to plan a surprise vacation for Alex.  Not knowing the implementation details of cookies, data stored on a server, and the way Firefox treats private browsing mode, she turns on private browsing mode and begins to research exotic tropical locations.  Using a cookie stored in her profile, Google Web History captures every search she does, not realizing that she is interested in privacy because she never logged out of her google account.  Alex sits down at Anna's computer and her Web History widget on her iGoogle home page exposes every search that she has done.

I can see users thinking that "privacy means privacy" and since they don't really get how all of this stuff works, situations like those two being rather common on shared computers.  This leads me to think that we should probably block access to cookies when the user is in private browsing mode.

Comment 55

[Shawn Wilsher :sdwilsh]

What about the situation where a user wonders why they just got logged out of their Amazon account?

Comment 56

[Alex Faaborg [:faaborg] (Firefox UX)]

>What about the situation where a user wonders why they just got logged out of
>their Amazon account?

Yeah, this is why beltzner was in favor of maintaining access to cookies, because people won't get why nothing works when they are in private browsing mode.

Could we block access to exiting cookies, but allow the creation of new cookies in memory?  This would at least enable users to log back into amazon.com.  If they have to do the action of logging in, then maybe they won't be so upset when information gets tracked?

Neither solution is perfect, but I personally am leaning towards erring on the side of privacy.

Comment 57

[(no longer active)]

(In reply to comment #54)
> I can see users thinking that "privacy means privacy" and since they don't
> really get how all of this stuff works, situations like those two being rather
> common on shared computers.  This leads me to think that we should probably
> block access to cookies when the user is in private browsing mode.

Hmm, interesting use cases.  Let's do an item by item analysis based on such a viewpoint.

We have the following items that should be affected by private browsing (feel free to add to the list if I'm missing anything):

* Cache: we disable disk and offline caches, and continue using the memory cache.  The memory cache may contain items specific to user's identity (for example, a page sent by a web application which is dynamically generated using user's cookie, and has stayed in the cache).  Therefore, we may want to clear the memory cache before proceeding in the private browsing mode as well.  The current implementation empties the memory cache before leaving the private browsing mode, so that no content from the private session lives in the memory after it.

* History: we simply disable adding new history items in the private browsing mode.  The previously existing history items are usable though.  The URLs stored in the history may contain session IDs, etc. (think of sites which embed session IDs in URLs instead of cookies), but disabling history completely does not seem like a good option.

* Form AutoComplete: we disable adding new form autocomplete items in private browsing mode, but using previously stored items is allowed.  I can't think of why this may be a possible privacy concern...

* Download Manager: we disable keeping downloaded items in the downloads history in the private browsing mode, but old items remain there, which looks innocent.

* Cookies: we currently allow cookies to be saved in the private browsing mode, and delete them once we're leaving this mode.  The entire collection of cookies is usable in private browsing mode.  This implementation is not good (see the issues in comment 21), but I have not yet worked on this part at all (I'm waiting on bug 411952...).  The implementation you suggest in comment 56 actually seems good enough: access to the existing cookies should be blocked, but new cookies should be able to be saved to and restored from memory while in private browsing mode.  Exiting this mode will delete the in-memory temporary cookies table, and will switch to the default set of cookies for use in normal browsing mode.  The implementation of such behavior is more difficult than the existing plan, but it may be worth it (I'm OK with the implementation challenge.)

* Session Store: the session store in private browsing mode is prevented from writing anything to disk, but maintains its own memory data collections.  Upon leaving this mode, the entire data collected throughout this mode is dumped, and the data is collected once again.  Upon entering the private browsing mode, the previously collected session store data is available (for example, you can undo closed tabs), which is no bigger a privacy risk than the previously stored history items being accessible.  (BTW, now that I think of it, Session Store should restore its exact data from the last snapshot before entering the private browsing mode, and then scan for newly added/changed tabs and windows.  This way the recently closed tabs at the start of the private browsing session are available after exiting it.  I'll implement this in a future version of the patch.)

* Password Manager: the password manager should not prompt to save new login info in the private browsing mode, but should be able to autofill form elements with previously stored info.  This is exactly like the Form History and I think it's OK.

* Permissions Manager: the permissions manager should be prevented from storing new permissions in the private browsing mode (because storing new permissions could reveal sites visited in the private browsing mode.)  Respecting previously stored permissions should be OK (like the case of Form History).  BTW I'm currently working on this, and I'll post a patch shortly.

* Certificate Manager: the certificate exceptions should not be stored in the private browsing mode (like the case for Permissions Manager).  Respecting previously stored exceptions should be OK (like the case of Permissions Manager).

* Add-ons Install Whitelist: The same goes here like Permissions Manager and Certificate Manager.  (BTW, wasn't add-ons install whitelist supposed to be eliminated in Firefox 3?  What is the status of it?  Should I invest time on working on it in this bug?)

* Error Console: the error console should be prevented from accepting messages while in private browsing mode, because many messages contain site URLs.  Viewing the previous messages from normal browsing modes should not be a privacy concern.

Comments/suggestions/criticisms are most welcome!

Comment 58

[(no longer active)]

Attachment: WIP Patch 6 - nsPermissionManager

This patch implements the backend changes in the nsPermissionManager, and also disabled the UI elements responsible for changing permissions in the browser code.  With this patch, when the user enters the private browsing mode, she cannot set any permissions on that site (because otherwise the fact that she has visited that site gets exposed later), but can consume the existing permissions.

The UI elements handled include:
* Firefox preferences window (the Exceptions buttons for images, cookies, and installs)
* Firefox notification box menu on pages with a popup
* Firefox permission editing controls in the Page Info dialog

Comment 59

[Shawn Wilsher :sdwilsh]

Here's an idea with cookies that I think might be ideal:
Copy the existing cookie table to a new table, like moz_old_cookies (better name needed).
Continue to use the existing cookie table, but once private browsing mode is exited, we copy the old table back into the new one and delete moz_old_cookies.

This means we get *all* of the cookies we had before when we enter, and then we go back to the exact state we were in before once we exit.

It might also be better to just copy the db file (probably faster).

Comment 60

[[e-mail]]

(In reply to comment #59)
> Here's an idea with cookies that I think might be ideal:

sdwilsh speaketh the truth. there are a few problems with your approach (patch 6):

1) if you use nsCookie::LastAccessed(), you're going to end up deleting cookies that were accessed by (i.e., sent back to) a webserver, but never modified. so you'll be overeager in your deletion.

2) if you use the cookie's creation time (approximately nsCookie::CreationID()), you'll catch cookies created since the private browsing epoch, but not cookies that were modified. creation times are not updated when a cookie's value is changed, which is important here. in addition, you'll allow last accessed times to be updated on sites that were visited, which (although pretty minor) would allow someone to tell what those sites were. (we don't currently expose that information on nsICookie{2}, but someone could sniff it from the db.)

3) i'm not sure i like the implementation of private browsing being pushed into the cookie/permission services, since these are part of the core netwerk module and are supposed to be as little app-specific as possible - but if it's well-written and minimal, i may be convinced.

a better approach would be to back up the original cookie and db files to a known location, do the private browsing, and then restore them. you'll have to figure out what to do in event of crash, though - we don't want that private data being left around, or orphaned.

Comment 61

[Justin Dolske [:Dolske]]

Comment on attachment 296738 [details] [diff] [review]
WIP Patch 6 - nsPermissionManager


>+            var prefBranch = Cc["@mozilla.org/preferences-service;1"].
>+                             getService(Ci.nsIPrefService).getBranch("privacy.");
>+            in_pb = prefBranch.getBoolPref("privatebrowsing");

Hmm, I wonder if this might be better implemented as an observer+topic.

Same net effect, just a conceptual difference.


>     promptAuth : function (aChannel, aLevel, aAuthInfo) {
...
>-            var notifyBox = this._getNotifyBox();
>-            if (notifyBox)
>-                this._removeSaveLoginNotification(notifyBox);
>+            var notifyBox = null;
>+            if (!inPrivateBrowsing) {
>+                var notifyBox = this._getNotifyBox();
>+                if (notifyBox)
>+                    this._removeSaveLoginNotification(notifyBox);
>+            }

This shouldn't change. If we're about to prompt for a login, any existing save-password notification bar should be removed to avoid confusion about what's being saved.


>             var canRememberLogin = this._pwmgr.getLoginSavingEnabled(hostname);
>-        
>+
>             // if checkboxLabel is null, the checkbox won't be shown at all.
>-            if (canRememberLogin && !notifyBox)
>+            if (canRememberLogin && !notifyBox && !inPrivateBrowsing)
>                 checkboxLabel = this._getLocalizedString("rememberPassword");

I think the logic here would be simpler if canRememberLogin was forced to |false| is private browsing mode is enabled.

Comment 62

[Shawn Wilsher :sdwilsh]

Comment on attachment 296738 [details] [diff] [review]
WIP Patch 6 - nsPermissionManager

>Index: toolkit/components/downloads/src/nsDownloadManager.cpp
>+  // If the user is in private browsing mode, delete the download record when
>+  // completed
>+  PRBool privMode = PR_FALSE;
>+  pref->GetBoolPref(PREF_PRIVATEBROWSING, &privMode);
>+  if (privMode)
>+    return 0;
>+
please grab the result of GetBoolPref, and in the if statement, check if it was successful as well.

Comment 63

[(no longer active)]

(In reply to comment #59)
> Here's an idea with cookies that I think might be ideal:
> Copy the existing cookie table to a new table, like moz_old_cookies (better
> name needed).
> Continue to use the existing cookie table, but once private browsing mode is
> exited, we copy the old table back into the new one and delete moz_old_cookies.
> 
> This means we get *all* of the cookies we had before when we enter, and then we
> go back to the exact state we were in before once we exit.
> 
> It might also be better to just copy the db file (probably faster).

See comment 32:

"By choosing to write *some* data to disk (perhaps in an encrypted format) we
have broken a clear and easy to understand contract between Firefox and the
user. The user / security expert will not be sure that there is no security
risk."

Mconnor: what do you think?

Comment 64

[(no longer active)]

(In reply to comment #60)
> (In reply to comment #59)
> > Here's an idea with cookies that I think might be ideal:
> 
> sdwilsh speaketh the truth. there are a few problems with your approach (patch
> 6):

Hmm, I haven't actually touched the cookie code in any significant way from Michael's original patch yet...

> 1) if you use nsCookie::LastAccessed(), you're going to end up deleting cookies
> that were accessed by (i.e., sent back to) a webserver, but never modified. so
> you'll be overeager in your deletion.

Yeah, and we don't want that.

> 2) if you use the cookie's creation time (approximately
> nsCookie::CreationID()), you'll catch cookies created since the private
> browsing epoch, but not cookies that were modified. creation times are not
> updated when a cookie's value is changed, which is important here. in addition,
> you'll allow last accessed times to be updated on sites that were visited,
> which (although pretty minor) would allow someone to tell what those sites
> were. (we don't currently expose that information on nsICookie{2}, but someone
> could sniff it from the db.)

Agreed.

> 3) i'm not sure i like the implementation of private browsing being pushed into
> the cookie/permission services, since these are part of the core netwerk module
> and are supposed to be as little app-specific as possible - but if it's
> well-written and minimal, i may be convinced.

Hmm, what do you think specifically about the implementation in the cache and permissions backends in patch 6 (attachment 296738 [details] [diff] [review])?

Based on the service which needs behavior modifications, there may be things which can be done from the UI.  For example, for the permissions service, my patch changes the nsPermissionManager slightly, and also disables any place in the UI which is used to add modify anything in nsPermissionManager.  But I'm not sure how a similar "higher-level" approach can be taken for cache and cookie services, since they are accessed by other modules in netwerk, and patching the UI won't work in those cases (at least, I can't see how it would work.)

> a better approach would be to back up the original cookie and db files to a
> known location, do the private browsing, and then restore them. you'll have to
> figure out what to do in event of crash, though - we don't want that private
> data being left around, or orphaned.

Hmm, IINM, as long as we write the cookies on disk (no matter in the original or in a new db file), we are risking the information to get leaked.  There is not much we can do in case of a crash, and there is even less that can be done in case Firefox is "end-tasked" without a chance to do any cleanup.  And by using sqlite as the backend, we ensure that the data is left in a usable state on the disk, so we can't even hope for it to get corrupted.  :-)

This is why I think an approach such as comment 56 is better in this regard (I'm talking about the having an in-memory DB part, not necessarily avoiding using the existing cookies.)

Comment 65

[(no longer active)]

(In reply to comment #62)
> (From update of attachment 296738 [details] [diff] [review])
> >Index: toolkit/components/downloads/src/nsDownloadManager.cpp
> >+  // If the user is in private browsing mode, delete the download record when
> >+  // completed
> >+  PRBool privMode = PR_FALSE;
> >+  pref->GetBoolPref(PREF_PRIVATEBROWSING, &privMode);
> >+  if (privMode)
> >+    return 0;
> >+
> please grab the result of GetBoolPref, and in the if statement, check if it was
> successful as well.

The idea here (and in other parts of the patch) is to assume that we are not in the safe browsing mode, and try to prove otherwise.  That is, if the GetBoolPref fails for any reason, privMode would remain PR_FALSE...  Is this assumption correct?  If not, then I would have change this (and possibly other places in the patch as well).

Comment 66

[(no longer active)]

Attachment: WIP Patch 7 - Fixes according to comment 61

Thanks for the comments, Justin.

(In reply to comment #61)
> (From update of attachment 296738 [details] [diff] [review])
> 
> >+            var prefBranch = Cc["@mozilla.org/preferences-service;1"].
> >+                             getService(Ci.nsIPrefService).getBranch("privacy.");
> >+            in_pb = prefBranch.getBoolPref("privatebrowsing");
> 
> Hmm, I wonder if this might be better implemented as an observer+topic.
> 
> Same net effect, just a conceptual difference.

See the first part of my comments in comment 32.  If I'm missing something here, I'd change the patch accordingly, but no one has commented on my reasoning yet...

> >     promptAuth : function (aChannel, aLevel, aAuthInfo) {
> ...
> >-            var notifyBox = this._getNotifyBox();
> >-            if (notifyBox)
> >-                this._removeSaveLoginNotification(notifyBox);
> >+            var notifyBox = null;
> >+            if (!inPrivateBrowsing) {
> >+                var notifyBox = this._getNotifyBox();
> >+                if (notifyBox)
> >+                    this._removeSaveLoginNotification(notifyBox);
> >+            }
> 
> This shouldn't change. If we're about to prompt for a login, any existing
> save-password notification bar should be removed to avoid confusion about
> what's being saved.

You're right, sorry for the mistake.

> >             var canRememberLogin = this._pwmgr.getLoginSavingEnabled(hostname);
> >-        
> >+
> >             // if checkboxLabel is null, the checkbox won't be shown at all.
> >-            if (canRememberLogin && !notifyBox)
> >+            if (canRememberLogin && !notifyBox && !inPrivateBrowsing)
> >                 checkboxLabel = this._getLocalizedString("rememberPassword");
> 
> I think the logic here would be simpler if canRememberLogin was forced to
> |false| is private browsing mode is enabled.

Done.

The new patch fixes the above two issues, and also adds browser/components/preferences/permissions.js which was accidentally dropped off from patch 6.

Comment 67

[[e-mail]]

(In reply to comment #63)
> This is why I think an approach such as comment 56 is better in this regard
> (I'm talking about the having an in-memory DB part, not necessarily avoiding
> using the existing cookies.)

hmm. in fact - you can avoid that problem by closing the db connection in the cookieservice, making further operations occur in-memory. on exiting private browsing mode, just reload the cookie table from disk (i.e. call InitDB()).

that should be very simple to implement - and requires only a few lines of code in nsCookieService::Observe(). for permissionmanager, you could do something similar.

Comment 68

[(no longer active)]

(In reply to comment #67)
> hmm. in fact - you can avoid that problem by closing the db connection in the
> cookieservice, making further operations occur in-memory. on exiting private
> browsing mode, just reload the cookie table from disk (i.e. call InitDB()).

Wow, I didn't know the cookie service works in this way.  So, if the DB connection is unavailable, will all operations (adding cookies, updating cookies, etc.) happen successfully in memory?  And will the full list of cookies read from the DB upon initialization remain intact (so that old cookies do not get lost when entering the private browsing mode)?

If the above is true, then it will indeed be very simple to implement.

> that should be very simple to implement - and requires only a few lines of code
> in nsCookieService::Observe(). for permissionmanager, you could do something
> similar.

Does the above apply to the permission manager as well?  Can I safely close the DB connection in the middle of its operation, and all ops from that point on get executed in memory, and later on when a new DB connection is opened, everything will revert back to just like it was before closing the connection?

Comment 69

[[e-mail]]

(In reply to comment #68)
> Wow, I didn't know the cookie service works in this way.  So, if the DB
> connection is unavailable, will all operations (adding cookies, updating
> cookies, etc.) happen successfully in memory?  And will the full list of
> cookies read from the DB upon initialization remain intact (so that old cookies
> do not get lost when entering the private browsing mode)?

indeed! the cookie service effectively has two data stores - on-disk and in-memory. on loading a profile, it reads data from the on-disk db into memory, and from that point on keeps them synced. (note that session cookies are held only in memory, and never see the platter - so the in-memory db is a superset of the on-disk one.)

simply closing that db connection at any point in time will cause it to run only from memory. it's designed that way - having a profile around to access is purely optional (think embeddors here).

> Does the above apply to the permission manager as well?  Can I safely close the
> DB connection in the middle of its operation, and all ops from that point on
> get executed in memory, and later on when a new DB connection is opened,
> everything will revert back to just like it was before closing the connection?

permission manager works identically. although by "opening a new connection" you're really clearing the existing in-memory table, and then calling InitDB() to reload the on-disk data.

Comment 70

[(no longer active)]

(In reply to comment #69)
> indeed! the cookie service effectively has two data stores - on-disk and
> in-memory. on loading a profile, it reads data from the on-disk db into memory,
> and from that point on keeps them synced. (note that session cookies are held
> only in memory, and never see the platter - so the in-memory db is a superset
> of the on-disk one.)
> 
> simply closing that db connection at any point in time will cause it to run
> only from memory. it's designed that way - having a profile around to access is
> purely optional (think embeddors here).

OK, I'm amused!  :-)  Thanks a lot for the pointer, Dan.  So, I guess this removes the dependency on bug 411952...

The only thing which needs to be decided before I can come up with a patch is if we want to keep the previously stored cookies around, or start from scratch as Alex suggested in comment 56.  Faaborg, Beltzner, Mconnor: any ideas?

> permission manager works identically. although by "opening a new connection"
> you're really clearing the existing in-memory table, and then calling InitDB()
> to reload the on-disk data.

Yeah, that is what we would want.  This makes the whole UI changes to disable setting new permissions introduced in patch 6 redundant as well: users *will* be able to set new permissions in the private browsing mode, only those new permissions would last until the end of their private browsing session, and would get vanished right after they exit the private browsing session.

BTW, this makes me think that the private browsing should not last between browser session restarts.  Think of what would happen to the cookies if the user closes the browser without exiting the private browsing mode...  They would get lost (because they are stored in memory.)  I think this gives me a clear idea why private browsing mode should not be retained after starting the browser.  If we have an indicator in the primary UI (like Alex suggested in comment 41) then this won't be much of an issue, because the user will know immediately that she's no longer in the private browsing mode after she restarts the browser.  And it makes the observer/notification implementation suggested by mconnor and Justin before viable.  We'll probably want a component which runs as a service which could be queried for the state of the private browsing mode, and can be used to initiate the browsing mode.  This component can be used for all higher level code (possibly excluding the netwerk/permission code, which can use raw observers.)

This sounds like quite a bit of work, but I'll try to come up with a new patch shortly.

Comment 71

[(no longer active)]

(In reply to comment #69)
> indeed! the cookie service effectively has two data stores - on-disk and
> in-memory. on loading a profile, it reads data from the on-disk db into memory,
> and from that point on keeps them synced. (note that session cookies are held
> only in memory, and never see the platter - so the in-memory db is a superset
> of the on-disk one.)

One more question here.  Would closing the connection automatically lead to the in-memory table to clear, or not?  If not, is there an easy way to clear it if needed?  This would be important when entering the private browsing mode.

Also, like what you mentioned about the permission manager, would re-opening the connection cause the in-memory table to be reloaded from the DB?  This would be important when exiting the private browsing mode.

Comment 72

[[e-mail]]

(In reply to comment #70)
> Yeah, that is what we would want.  This makes the whole UI changes to disable
> setting new permissions introduced in patch 6 redundant as well: users *will*
> be able to set new permissions in the private browsing mode, only those new
> permissions would last until the end of their private browsing session, and
> would get vanished right after they exit the private browsing session.

i like this approach much better!

(In reply to comment #71)
> One more question here.  Would closing the connection automatically lead to the
> in-memory table to clear, or not?  If not, is there an easy way to clear it if
> needed?  This would be important when entering the private browsing mode.

it won't - see {nsCookieService,nsPermissionManager}::RemoveAllFromMemory(). i'd be in favor of not doing this, though the UX guys might think it's best so that users have the "feel" of things being reset/private.

closing the db connection is as simple as nulling out mDBConn.

> Also, like what you mentioned about the permission manager, would re-opening
> the connection cause the in-memory table to be reloaded from the DB?  This
> would be important when exiting the private browsing mode.

it won't ;) you'll have to call {nsCookieService,nsPermissionManager}::InitDB().

one consideration here, specific to cookies: say the user starts the browser (loading the cookie db from disk), and then browses around for a while (accumulating session cookies in memory, that never get written to disk). they now have both persistent and session cookies in memory. then they enter PB mode, and accumulate more cookies. on exiting that mode, we throw away everything and reload persistent cookies from disk. those session cookies they acquired before entering PB mode are now lost. (also consider the case where the user has 'limit cookie lifetime to session' enabled, so all their cookies are session-only.) do we care about these cases? phrased another way, is it acceptable for exiting PB mode to behave like a browser restart?

Comment 73

[(no longer active)]

(In reply to comment #72)
> (In reply to comment #70)
> > Yeah, that is what we would want.  This makes the whole UI changes to disable
> > setting new permissions introduced in patch 6 redundant as well: users *will*
> > be able to set new permissions in the private browsing mode, only those new
> > permissions would last until the end of their private browsing session, and
> > would get vanished right after they exit the private browsing session.
> 
> i like this approach much better!

Me too.  :-)

> (In reply to comment #71)
> > One more question here.  Would closing the connection automatically lead to the
> > in-memory table to clear, or not?  If not, is there an easy way to clear it if
> > needed?  This would be important when entering the private browsing mode.
> 
> it won't - see {nsCookieService,nsPermissionManager}::RemoveAllFromMemory().
> i'd be in favor of not doing this, though the UX guys might think it's best so
> that users have the "feel" of things being reset/private.
> 
> closing the db connection is as simple as nulling out mDBConn.

Thanks for the tips!

> > Also, like what you mentioned about the permission manager, would re-opening
> > the connection cause the in-memory table to be reloaded from the DB?  This
> > would be important when exiting the private browsing mode.
> 
> it won't ;) you'll have to call
> {nsCookieService,nsPermissionManager}::InitDB().

Hmm, nsCookieService::InitDB() calls nsCookieService::Read() which leads to items in the DB be read into the memory table, right?

> one consideration here, specific to cookies: say the user starts the browser
> (loading the cookie db from disk), and then browses around for a while
> (accumulating session cookies in memory, that never get written to disk). they
> now have both persistent and session cookies in memory. then they enter PB
> mode, and accumulate more cookies. on exiting that mode, we throw away
> everything and reload persistent cookies from disk. those session cookies they
> acquired before entering PB mode are now lost. (also consider the case where
> the user has 'limit cookie lifetime to session' enabled, so all their cookies
> are session-only.) do we care about these cases? phrased another way, is it
> acceptable for exiting PB mode to behave like a browser restart?

Hmm, would it be possible to get a copy of the in-memory hash table of cookies, and restore it when exiting from the private browsing mode, instread of calling nsCookieService::RemoveAllFromMemory()?  This way, everything would get restored to what it was exactly before initiating the private browsing session.

Comment 74

[[e-mail]]

(In reply to comment #73)
> Hmm, nsCookieService::InitDB() calls nsCookieService::Read() which leads to
> items in the DB be read into the memory table, right?

right.

> Hmm, would it be possible to get a copy of the in-memory hash table of cookies,
> and restore it when exiting from the private browsing mode, instread of calling
> nsCookieService::RemoveAllFromMemory()?  This way, everything would get
> restored to what it was exactly before initiating the private browsing session.

yes, though that'd be a little more complicated. we should probably decide how we want this to behave before embarking on that ;)

Comment 75

[Shawn Wilsher :sdwilsh]

(In reply to comment #65)
> The idea here (and in other parts of the patch) is to assume that we are not in
> the safe browsing mode, and try to prove otherwise.  That is, if the
> GetBoolPref fails for any reason, privMode would remain PR_FALSE...  Is this
> assumption correct?  If not, then I would have change this (and possibly other
> places in the patch as well).
XPCOM rules say you should not trust an out parameter if the method did not return successfully.

Comment 76

[(no longer active)]

(In reply to comment #74)
> (In reply to comment #73)
> > Hmm, nsCookieService::InitDB() calls nsCookieService::Read() which leads to
> > items in the DB be read into the memory table, right?
> 
> right.
> 
> > Hmm, would it be possible to get a copy of the in-memory hash table of cookies,
> > and restore it when exiting from the private browsing mode, instread of calling
> > nsCookieService::RemoveAllFromMemory()?  This way, everything would get
> > restored to what it was exactly before initiating the private browsing session.
> 
> yes, though that'd be a little more complicated. we should probably decide how
> we want this to behave before embarking on that ;)

OK, waiting for comments from UX guys...

Comment 77

[(no longer active)]

(In reply to comment #75)
> XPCOM rules say you should not trust an out parameter if the method did not
> return successfully.

Oh, thanks for mentioning this.  I'll post an updated patch shortly.

Comment 78

[Alex Faaborg [:faaborg] (Firefox UX)]

> > Hmm, would it be possible to get a copy of the in-memory hash table of cookies,
> > and restore it when exiting

>OK, waiting for comments from UX guys...

To make sure I fully understand the question, what are the interface implications of restoring the in-memory cookies from before entering private browsing mode.  Would this enable use to let the user enter and exit without having to end their session?

Comment 79

[Alex Faaborg [:faaborg] (Firefox UX)]

>The only thing which needs to be decided before I can come up with a patch is
>if we want to keep the previously stored cookies around, or start from scratch
>as Alex suggested in comment 56.  Faaborg, Beltzner, Mconnor: any ideas?

It would be good if beltzner or mconnor weighed in as well, but after thinking about this some more, I believe users will be surprised if sites recognize them after entering private browsing mode.  If you think of private browsing mode as putting on a mask, than the first thing that happens is Amazon.com says "Hello Alex Faaborg!" that implies the mask isn't very good.

Comment 80

[(no longer active)]

(In reply to comment #78)
> >OK, waiting for comments from UX guys...
> 
> To make sure I fully understand the question, what are the interface
> implications of restoring the in-memory cookies from before entering private
> browsing mode.  Would this enable use to let the user enter and exit without
> having to end their session?

There are two options to implement private browsing -- cookie-wise:

1. Give the user a "clean" session by not using any previously used cookies whatsoever.  This is just like the case where they create a new profile and use it to browse the web.  The cookies can be stored during user's private browsing session.  After the user exits, any cookies stored will be discarded.  All cookies are stored only in memory, so no track of user's actions are saved to disk.

2. Just like case 1, with the exception that the previously stored cookies are not discarded; they're used as before, but no update on the cookies will be stored on disk, and the exact snapshot of the in-memory cookie table before entering the private browsing mode would be available afterward, as if no private browsing has ever occurred.

Comment 81

[(no longer active)]

(In reply to comment #79)
> It would be good if beltzner or mconnor weighed in as well, but after thinking
> about this some more, I believe users will be surprised if sites recognize them
> after entering private browsing mode.  If you think of private browsing mode as
> putting on a mask, than the first thing that happens is Amazon.com says "Hello
> Alex Faaborg!" that implies the mask isn't very good.

Or Amazon is too good!  ;-)

Seriously, the more I think about it, the more logical your approach seems to me...

Comment 82

[(no longer active)]

Attachment: WIP Patch 8 - Rewrite using notification/observers (not completed yet)

Here is a new version of the patch, that I've rewritten from scratch.  This version replaces the "privacy.privatebrowsing" pref with the "browser:private-browsing" notification.  This notification gets sent by the gPrivateBrowsingMgr object defined in browser.js.  The data value of this notification determines if we're entering the private browsing mode or leaving it.  I have added a quick and dirty item to the Tools menu to toggle the private browsing mode to aid testing, and this will be removed in the final version of the patch, as the UI for private browsing mode would be implemented in bug 411929.

This patch implements the changes to the cache, history, form fill history and download manager components.  The changes to the cookies, session store, login manager, permission manager, and error console components will be implemented shortly in a future WIP patch.  For now, I'm assuming that we want the cookie behavior that Alex suggested earlier, unless I hear otherwise from him or mconnor/beltzner.

Comment 83

[(no longer active)]

Sorry to CC you again, Simon.

I was wondering if it's possible to restore the SessionStore service after the private browsing mode in a way that the SessionStore data for recently closed tabs is exactly like what it was right before entering the private browsing mode.

Here's the use case:
1) John is browsing the web.  His recently closed tabs list includes [Google, Gmail, CNN].
2) He enters private browsing mode, opens a new tab to browse Amazon, and he closes it later on.  His recently closed tabs list at this point: [Google, Gmail, CNN, Amazon].
3) He exits private browsing mode.

With the current implementation, after 3 the recently closed tabs list is [].  I like to restore it to [Google, Gmail, CNN], but I'm not sure how to go about it and not hurt other parts of the code...

Suggestions welcome.  Thanks!

Comment 84

[(no longer active)]

(In reply to comment #75)
> (In reply to comment #65)
> > The idea here (and in other parts of the patch) is to assume that we are not in
> > the safe browsing mode, and try to prove otherwise.  That is, if the
> > GetBoolPref fails for any reason, privMode would remain PR_FALSE...  Is this
> > assumption correct?  If not, then I would have change this (and possibly other
> > places in the patch as well).
> XPCOM rules say you should not trust an out parameter if the method did not
> return successfully.

BTW, since I'm no longer using a pref, this should no longer be relevant.

Comment 85

[Simon Bünzli]

(In reply to comment #83)
> in a way that the SessionStore data for recently closed tabs is exactly
> like what it was right before entering the private browsing mode.

When entering private browsing mode, iterate through all _windows and for each of them clone the _closedTabs array into _closedTabsBackup (where cloning is best achieved through |eval(closedTabs.toSource())|). Then you can just move that backup back when you exit private browsing mode and delete _closedTabsBackup (and make sure that _closedTabs is still no larger than what .max_tabs_undo dictates).

Comment 86

[(no longer active)]

Attachment: WIP Patch 9 - Nearly complete

This patch is a followup to attachment 297698 [details] [diff] [review].

What's new:

* Cookie service: the implementation follows Dan's hint in comment 67, and Alex's idea in comment 54.  The in-memory DB would be restored when exiting the private browsing mode, which addresses an issue Dan raised in comment 72.

* Permission manager: the implementation has been changed to follow Dan's hint in comment 67.

* Login prompter: the implementation uses a separate object with greater lifetime than that of prompter objects, so that the true state of private browsing can be obtained at any time.

* Content prefs: no prefs will be saved during the private browsing mode, which makes it impossible to track user actions in private browsing mode based on the content prefs DB.

* Session store: added the implementation Simon mentioned in comment 85.  Note: this does not work as intended (i.e., the Recently closed tabs menu item is grayed out after exiting the private browsing mode).  Need to investigate this further.

* Console service: added a simple mechanism to pause/resume recording of messages in the console service, and modified Private Browsing Manager in browser.js to use this facility to make sure nothing is logged to the console service during the private browsing session.  I didn't modify the service itself to observe browser:private-browsing notifications because that could lead to circular calls made back to the service by the observer service.

* Private Browsing manager: handled quit-application-granted, to make sure to exit private browsing mode just before application quit, so that services which need cleanup can get the proper exit notification.

The only other service which needs modification is the certificate exceptions list.  I'll implement that in a next patch, and will ask for review on various parts of the patch then.  If anyone can think of other services which need modification for private browsing mode, please let me know.

Comment 87

[(no longer active)]

Attachment: WIP Patch 9 - Nearly complete

Attachment 297997 [details] [diff], updated to trunk.

Comment 88

[Simon Bünzli]

(In reply to comment #86)
> * Session store: added the implementation Simon mentioned in comment 85.

I'll review the changes when you're done. Just a note on your issue: it doesn't work because of the splicing in _restoreRecentlyClosedTabs. Instead you'll have to |.slice(0, maxTabsUndo)|...

> * Console service: added a simple mechanism to pause/resume recording of
> messages in the console service

I'd rather see an attribute "paused" (or "logging", "enabled" or something alike) than two methods "pause" and "resume" so that e.g. Firebug can easily determine whether it should inform the user that she shouldn't be expecting any errors in the console (without having to keep track of private browsing mode).

> * Private Browsing manager: handled quit-application-granted, to make sure to
> exit private browsing mode just before application quit

That code will have to move into an XPCOM component (e.g. nsBrowserGlue) or it'll fail to run when the last closed window isn't a browser one.

Comment 89

[(no longer active)]

Attachment: WIP Patch 10 - Fixes according to comment 88

(In reply to comment #88)
> (In reply to comment #86)
> > * Session store: added the implementation Simon mentioned in comment 85.
> 
> I'll review the changes when you're done. Just a note on your issue: it doesn't
> work because of the splicing in _restoreRecentlyClosedTabs. Instead you'll have
> to |.slice(0, maxTabsUndo)|...

I should be ashamed of not catching this myself.  Of course you were absolutely right!  Fixed.

> > * Console service: added a simple mechanism to pause/resume recording of
> > messages in the console service
> 
> I'd rather see an attribute "paused" (or "logging", "enabled" or something
> alike) than two methods "pause" and "resume" so that e.g. Firebug can easily
> determine whether it should inform the user that she shouldn't be expecting any
> errors in the console (without having to keep track of private browsing mode).

Done.  The new patch uses the |paused| attribute on the console service.

> > * Private Browsing manager: handled quit-application-granted, to make sure to
> > exit private browsing mode just before application quit
> 
> That code will have to move into an XPCOM component (e.g. nsBrowserGlue) or
> it'll fail to run when the last closed window isn't a browser one.

I moved the whole stuff in browser.js to its own XPCOM component in browser/components.

Comment 90

[Mike Beltzner [:beltzner, not reading bugmail]]

Ehsan - there's a lot of great work in here. Any chance of you being able to put this all together in an XPI to get broader testing of the functionality?

Comment 91

[(no longer active)]

(In reply to comment #90)
> Ehsan - there's a lot of great work in here. Any chance of you being able to
> put this all together in an XPI to get broader testing of the functionality?

I'd love to, but I'm not sure how to do it, since the code touches a lot of core components, such as cache, cookies, permissions, etc.  Maybe it would be better to provide builds with this patch applied for users to test?  I can provide Windows and Linux builds, but I need help in creating a Mac OS X build because I don't have access to any Mac computer...

Let me know if providing build would serve this purpose.

Comment 92

[[e-mail]]

(In reply to comment #91)
> but I need help in creating a Mac OS X build

"buildbot try" can roll builds on all the major plats for you, and make them available for download - check out build.mozilla.org when you have your cvs account set up. in the meantime, if you attach a patch here, someone can probably do it for you ;)

Comment 93

[(no longer active)]

(In reply to comment #92)
> "buildbot try" can roll builds on all the major plats for you, and make them
> available for download - check out build.mozilla.org when you have your cvs
> account set up. in the meantime, if you attach a patch here, someone can
> probably do it for you ;)

Thanks for the tip.  I guess I'll ask someone to make the builds on this bug as soon as I'm finished with the patch here.  I'm currently stuck by some bug in the cert override service, which I'm going to file shortly... :(

Comment 94

[Sylvain Pasche]

Having unit tests would also help integration I guess. I was thinking of
something like this:
Setup a set of pages running on httpd.js for testing cookies, auto-completion,
form entry and others. Then develop a browser chrome test
(http://developer.mozilla.org/en/docs/Browser_chrome_tests) which will simulate
navigation and use features affecting the profile state (browse to pages
setting cookies, simulate a password entry, simulate downloading a file, ...).

This simulated session could be run a first time with no private browsing mode
and check that all works well (cookies saved, auto-completion saved, ...). Then
we sanitize the profile, toggle private browsing mode and run the navigation
simulation again and the toggle private browsing mode off. After that, we check
that nothing is saved in the profile.

I can imagine two ways of checking that nothing is saved in the profile while
in private browsing mode:
1) one high level check that would ensure that no files were written during
private private browsing mode by checking file last write time (hoping this
works in a cross platform way). There could be a white list for things that are
allowed to change and don't introduce a privacy risk (like
urlclassifier3.sqlite I guess). The advantage of this kind of test is that it
could detect new features added without them managing the private browsing
mode, preventing privacy leaks added in the future.
2) Service specific checks (checking cookies, cache, form history, ...)

I don't think something similar already exist for the sanitizer service (clear
private data). Such a framework if developed could then test both the private
browsing mode and the sanitizer. For testing the sanitizer, we launch the
simulated browsing, run the sanitizer and check that nothing is saved in test 2
above (test 1 would not be effective, as files are written during navigation).

That was some ideas I had for making private browsing mode more robust. Of
course it's certainly more easy to write down than to implement ;-)


Maybe another service that could participate in the private browsing mode is
the site specific preferences? If you navigate to a site and change the zoom
level this information is saved in this store, thus revealing you visited that
site. This could use the same handling as the cookies: site specific
preferences are used as normal while in private browsing mode, but the store is
not updated in this mode (this means your zoom settings are not reset in
private browsing mode for instance).

Comment 95

[(no longer active)]

Thanks for your input, Sylvian.

(In reply to comment #94)
> Having unit tests would also help integration I guess. I was thinking of
> something like this:
> Setup a set of pages running on httpd.js for testing cookies, auto-completion,
> form entry and others. Then develop a browser chrome test
> (http://developer.mozilla.org/en/docs/Browser_chrome_tests) which will simulate
> navigation and use features affecting the profile state (browse to pages
> setting cookies, simulate a password entry, simulate downloading a file, ...).

Hmmm good idea.  I'm totally new in writing chrome tests, though.  I'm going through the docs on MDC right now, but if anyone wants to jump in and help on this, I'd be more than happy to accept inputs/hints/written tests.  :-)

> I don't think something similar already exist for the sanitizer service (clear
> private data). Such a framework if developed could then test both the private
> browsing mode and the sanitizer. For testing the sanitizer, we launch the
> simulated browsing, run the sanitizer and check that nothing is saved in test 2
> above (test 1 would not be effective, as files are written during navigation).

That would be part of another bug, right?  Maybe you could file one to keep it on-track?

> Maybe another service that could participate in the private browsing mode is
> the site specific preferences? If you navigate to a site and change the zoom
> level this information is saved in this store, thus revealing you visited that
> site.

I added this component in the WIP Patch 9 (attachment 297997 [details] [diff] [review]).

> This could use the same handling as the cookies: site specific
> preferences are used as normal while in private browsing mode, but the store is
> not updated in this mode (this means your zoom settings are not reset in
> private browsing mode for instance).

This implementation is actually quite complicated.  It was easy in case of cookies and the permission manager because they already provided an in-memory DB which is a superset of the on-disk DB.  The contentprefs service, however, does not even perform in-memory caching, and it reads and writes all the prefs directly from/to the DB.  And we have made similar tradeoffs in other services.  For example, the pages visited in the private browsing mode don't end up in your history even when you have not left the private browsing mode.  I think a similar tradeoff here would be acceptable, wouldn't it?

Comment 96

[Sylvain Pasche]

(In reply to comment #95)
> 
> That would be part of another bug, right?  Maybe you could file one to keep it
> on-track?

Yes, I think so. I'll do some research if it is filed already and enter a new one if needed.

> I added this component in the WIP Patch 9 (attachment 297997 [details] [diff] [review]).

Sorry I didn't see it while skimming through the patch.

> This implementation is actually quite complicated.  It was easy in case of
> cookies and the permission manager because they already provided an in-memory
> DB which is a superset of the on-disk DB.  The contentprefs service, however,
> does not even perform in-memory caching, and it reads and writes all the prefs
> directly from/to the DB.  And we have made similar tradeoffs in other services.
>  For example, the pages visited in the private browsing mode don't end up in
> your history even when you have not left the private browsing mode.  I think a
> similar tradeoff here would be acceptable, wouldn't it?

Seems acceptable to me. If I understand the patch well, the content preferences can be read but not written in private browsing mode. This means a site where you raised the zoom level will still be zoomed in private browsing mode as expected.

Comment 97

[(no longer active)]

(In reply to comment #96)
> (In reply to comment #95)
> > 
> > That would be part of another bug, right?  Maybe you could file one to keep it
> > on-track?
> 
> Yes, I think so. I'll do some research if it is filed already and enter a new
> one if needed.

Thanks!  Please CC me if you find an existing one (or file a new bug).

> Seems acceptable to me. If I understand the patch well, the content preferences
> can be read but not written in private browsing mode. This means a site where
> you raised the zoom level will still be zoomed in private browsing mode as
> expected.

Yes, exactly.

Comment 98

[(no longer active)]

Attachment: WIP Patch 11 - nsCertOverrideService

This new patch makes nsCertOverrideService honor private browsing mode.  The approach used is to store the overrides in memory while we are in private browsing mode, and clear them up after exiting this mode.  Nothing will be written to disk, even if the override entry is set to permanent mode.

Note that I have not been able to test this because of bug 413627.  I'll wait for some time and try to figure out a solution.  Then I'll ask for review on this patch.  Please let me know if you know how to solve this problem.  For more info, see bug 413627 comment 0.

Comment 99

[(no longer active)]

BTW, according to comment 92, if anyone can help build a test version for all the three platforms, that would be great.  Please use the WIP 11 patch for this purpose.

Comment 100

[(no longer active)]

Oops, I forgot to mention that I missed browser/components/Makefile.in in my WIP 9 and 10 patches, which would cause the privatebrowsing XPCOM component not to get built.  This is fixed in WIP 11 as well.

Comment 101

[Håkan Waara]

I gave the WIP 11 patch to the try-server, so a private browsing-build for all three platforms should appear in about 1 hour and 30 minutes here: https://build.mozilla.org/tryserver-builds/

Comment 102

[Sylvain Pasche]

(In reply to comment #97)
> Thanks!  Please CC me if you find an existing one (or file a new bug).

I filed bug 413659 for this.

Comment 103

[(no longer active)]

(In reply to comment #101)
> I gave the WIP 11 patch to the try-server, so a private browsing-build for all
> three platforms should appear in about 1 hour and 30 minutes here:
> https://build.mozilla.org/tryserver-builds/

Thanks!  Windows builds are already available in <https://build.mozilla.org/tryserver-builds/2008-01-23_05:[e-mail]-private_browsing_wip11/>.

Comment 104

[Håkan Waara]

(In reply to comment #103)
> (In reply to comment #101)
> > I gave the WIP 11 patch to the try-server, so a private browsing-build for all
> > three platforms should appear in about 1 hour and 30 minutes here:
> > https://build.mozilla.org/tryserver-builds/
> 
> Thanks!  Windows builds are already available in
> <https://build.mozilla.org/tryserver-builds/2008-01-23_05:[e-mail]-private_browsing_wip11/>.

Both Linux and OS X had build failures. See http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTry

Comment 105

[(no longer active)]

Attachment: WIP Patch 12 - Fixes according to comment 104

(In reply to comment #104)
> Both Linux and OS X had build failures. See
> http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTry

Oops.  This should fix the problem.  Can you submit a new try server build?

Comment 106

[(no longer active)]

Attachment: WIP Patch 12 - Fixes according to comment 104

Oops, the previous patch accidentally contained some other code that I'm working on...  This one should really fix the problem :-)

Comment 107

[Shawn Wilsher :sdwilsh]

I submitted it - should show up in the same place when it's all set.

Comment 108

[(no longer active)]

(In reply to comment #107)
> I submitted it - should show up in the same place when it's all set.

Thanks!  Windows and Mac builds have finished: <https://build.mozilla.org/tryserver-builds/2008-01-23_10:[e-mail]-Bug_248970/>.  Linux is still building.  I'll monitor the tinderbox to see if it succeeds.

And, I'm preparing the final version of the patch which is ready for review.  It should be ready in a few minutes.  This version of the patch corrects the nsCertOverrideService implementation, and now it actually works!  It would be great if someone could submit another try build once I submit that patch.

Comment 109

[(no longer active)]

Attachment: Patch (v1)

OK, I think this is ready for review.

Changes in this version: the nsCertOverrideService implementation has been corrected as follows:

* I used NS_WITH_PROXIED_SERVICE() to proxy the calls to the nsIObserverService to the main thread, so that they won't fail, and notifications are received.

* I implemented nsISupportsWeakReference so that calls to nsIObserverService::AddObserver() with the last param set to true succeed.

(Note that this effectively fixes bug 413627 as well.)

-----

Note to reviewers:

1. The changes to browser-menubar.inc should not be reviewed.  Once this is ready to land, I'll remove the changes to that file.  Those are only meant to ease your life in testing the patch.

2. If you feel you won't be able to review this soon, please let me know to ask someone else for review.

-----

Asking Mike Connor to review the following parts:
toolkit/components/places/src/nsNavHistory.{h,cpp}
toolkit/components/contentprefs/src/nsContentPrefService.js
browser/components/privatebrowsing

Asking Benjamin Smedberg for sr on the following parts:
netwerk/{cache,cookie}/*
extensions/cookie/*
security/manager/ssl/*
xpcom/base/*

Asking Mike Beltzner for ui-r based on the descriptions in the previous comments.

Comment 110

[(no longer active)]

CCing the relevant people from bug 413627...

Comment 111

[(no longer active)]

Comment on attachment 298757 [details] [diff] [review]
Patch (v1)

Asking Benjamin Smedberg for review on the following parts:

xpcom/base/nsIConsoleService.idl
xpcom/base/nsConsoleService.{h,cpp}

Comment 112

[(no longer active)]

Comment on attachment 298757 [details] [diff] [review]
Patch (v1)

Asking Simon Bünzli for review on the following parts:

browser/components/sessionstore/src/nsSessionStore.js

Comment 113

[(no longer active)]

Comment on attachment 298757 [details] [diff] [review]
Patch (v1)

Asking Christian Biesinger for review on the following parts:

netwerk/cache/src/nsCacheService.{h,cpp}

Comment 114

[(no longer active)]

Comment on attachment 298757 [details] [diff] [review]
Patch (v1)

Asking Dan Witte for review on the following parts:

netwerk/cookie/src/nsCookieService.{h,cpp}
extensions/cookie/nsPermissionManager.{h,cpp}

Comment 115

[(no longer active)]

Comment on attachment 298757 [details] [diff] [review]
Patch (v1)

Asking Bob Relyea for review on the following parts:

security/manager/ssl/src/nsCertOverrideService.{h,cpp}

Comment 116

[(no longer active)]

Comment on attachment 298757 [details] [diff] [review]
Patch (v1)

Asking Asaf Romano for review on the following parts:

toolkit/components/satchel/src/nsStorageFormHistory.{h,cpp}

Comment 117

[(no longer active)]

Comment on attachment 298757 [details] [diff] [review]
Patch (v1)

Asking Shawn Wilsher for review on the following parts:

toolkit/components/downloads/src/nsDownloadManager.{h,cpp}

Comment 118

[(no longer active)]

Comment on attachment 298757 [details] [diff] [review]
Patch (v1)

Asking Justin Dolske for review on the following parts:

toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js

(Oh, boy, I really wish Bugzilla could handle multiple review requests...)

Comment 119

[(no longer active)]

Targetting beta3...

Comment 120

[Reed Loden [:reed]]

(In reply to comment #118)
> (Oh, boy, I really wish Bugzilla could handle multiple review requests...)

It can. Just comma-separate the addresses. :)

Comment 121

[(no longer active)]

(In reply to comment #120)
> It can. Just comma-separate the addresses. :)

And if only I knew it 11 comments ago...  ;-)

Thanks for the tip, Reed!

Comment 122

[Kai Engert (:KaiE:)]

I must say, the approach implemented in the patch concerns me. You are modifying code in a lot of modules and try to prevent it from saving.

I think, using this approach, we will have to play "catch up" forever.
Whenever someone implements storage for some new data, you will have to enhance that storage logic to consider the proposed new mode.

I'm also concerned that it complicates the code.


I would like to bring up 2 alternative ideas.

a) Integrate your "private surfing mode" with profile manager. In other words, if the user chooses to surf privately, create a new profile. Mark that profile as such. During app shutdown, erase the profile. At worst the app crashes, so you could auto-cleanup the profile on next startup.

But a) does not work if you would like to prevent that data reaches the disk. In that case one could think of the following:


b) Maybe we could use an in-memory-virtual-filesystem? Implement file I/O that simulates a profile directory and files that live in RAM? Start a profile with a directory stored on that filesystem. This mechanism would guarantee we will never forget to adjust code that potentially stores sensitive data on disk (and keeps the code simple).

Comment 123

[(no longer active)]

(In reply to comment #122)
> I must say, the approach implemented in the patch concerns me. You are
> modifying code in a lot of modules and try to prevent it from saving.
> 
> I think, using this approach, we will have to play "catch up" forever.
> Whenever someone implements storage for some new data, you will have to enhance
> that storage logic to consider the proposed new mode.

That's correct, but things like bug 413659 could help remedy the problem (see comment 94).

> I'm also concerned that it complicates the code.
> 
> 
> I would like to bring up 2 alternative ideas.
> 
> a) Integrate your "private surfing mode" with profile manager. In other words,
> if the user chooses to surf privately, create a new profile. Mark that profile
> as such. During app shutdown, erase the profile. At worst the app crashes, so
> you could auto-cleanup the profile on next startup.
> 
> But a) does not work if you would like to prevent that data reaches the disk.
> In that case one could think of the following:

Yes, (a) won't work for private browsing.

> b) Maybe we could use an in-memory-virtual-filesystem? Implement file I/O that
> simulates a profile directory and files that live in RAM? Start a profile with
> a directory stored on that filesystem. This mechanism would guarantee we will
> never forget to adjust code that potentially stores sensitive data on disk (and
> keeps the code simple).

That's a good idea as well, but I'm wondering how much could should be touched to implement that one.  I'm not quite familiar with Mozilla's I/O abstraction layers, but if this needs to be done at a level similar to nsILocalFile, then I'm afraid that the same amount of changes need to be made to these modules, to switch to an alternative file implementation.  Others may be able to comment on this more precisely.

Comment 124

[Robert Relyea]

Comment on attachment 298757 [details] [diff] [review]
Patch (v1)

Switching review of security/manager/ssl/src/nsCertificateOverrideService.{h,cpp} since he's the original author of the code that is modified.

Kai feel free to kick it back to me if you can't get to the review.

Ehsan, You man also want to open NSS Read/Only. That will prevent manual changes and new Root cert imports (or even private key/user cert importing) from happenning in private sessions.

bob

Comment 125

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #119)
> Targetting beta3...

Just to set expectations properly, I don't think that we should count on this feature making it into Firefox 3. I say this because ..:

 - this bug isn't marked blocking- nor wanted-firefox3+
 - QA hasn't been planning on testing this mode
 - all of the reviewers will be focusing on blocking bugs first
 - some of Kai's comments make me think that there might be more thinking to be done here

This is great stuff, and I think a fantastic and huge step towards a much desired ability to do true private browsing. I'm looking forward to playing with the tryserver builds, and am truly appreciative of all the work put into this, and by no means am suggesting we abandon things. Just that we make sure to not get disappointed if it misses.

(That's why I was asking about the ability to get this done as an add-on, though I understand that might be difficult due to the nature of the changes - tryserver builds help!)

Comment 126

[(no longer active)]

(In reply to comment #124)
> (From update of attachment 298757 [details] [diff] [review])
> Switching review of
> security/manager/ssl/src/nsCertificateOverrideService.{h,cpp} since he's the
> original author of the code that is modified.
> 
> Kai feel free to kick it back to me if you can't get to the review.
> 
> Ehsan, You man also want to open NSS Read/Only. That will prevent manual
> changes and new Root cert imports (or even private key/user cert importing)
> from happenning in private sessions.

Hmmm, I think private browsing is about not collecting data automatically about user's private session, but some things should be allowed anyways, such as bookmarking, saving a web page, etc.  Therefore I doubt that we should go as far as opening NSS read-only...

(In reply to comment #122)
> b) Maybe we could use an in-memory-virtual-filesystem? Implement file I/O that
> simulates a profile directory and files that live in RAM? Start a profile with
> a directory stored on that filesystem. This mechanism would guarantee we will
> never forget to adjust code that potentially stores sensitive data on disk (and
> keeps the code simple).

Another thing which needs clarification here is that whether we want to give user a new profile, therefore depriving them of everything useful in their real profiles?  I'd expect users to want access to their bookmarks, or their custom search engines for example n private browsing mode, to get started browsing...

Comment 127

[Mike Beltzner [:beltzner, not reading bugmail]]

I resubmitted the latest patch to the tryserver, and am playing with the current one for now ...

Comment 128

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #126)
> Hmmm, I think private browsing is about not collecting data automatically about
> user's private session, but some things should be allowed anyways, such as
> bookmarking, saving a web page, etc.  Therefore I doubt that we should go as
> far as opening NSS read-only...

I think that's right. Saving web pages, adding bookmarks, and other explicit user actions asking to save data should be honoured. It's the automatic stuff we do to make the browser more usable: retaining history, saving form history, accepting/sending cookies ... that's the sort of stuff we want to restrict.

> Another thing which needs clarification here is that whether we want to give
> user a new profile, therefore depriving them of everything useful in their real
> profiles?  I'd expect users to want access to their bookmarks, or their custom
> search engines for example n private browsing mode, to get started browsing...

That's a really easy way to get a bare-bones private browsing mode installed, but it isn't very functional.

I'd rather see Kai's concerns addressed while maintaining this approach, fwiw.

Comment 129

[Simon Bünzli]

(In reply to comment #122)
> I think, using this approach, we will have to play "catch up" forever.

Not only us. What about an extension which saves urls to prefs (e.g. NoScript) or to their own data file (e.g. Adblock Plus and Tab Mix Plus)? Will somebody from MoCo go through AMO and make sure that that's taken care of for all extensions - or will private browsing only be really supported for extension-less profiles after all (just to be sure)?

Comment 130

[(no longer active)]

(In reply to comment #125)
> (In reply to comment #119)
> > Targetting beta3...

Thanks for the supportive comments, Mark!

> Just to set expectations properly, I don't think that we should count on this
> feature making it into Firefox 3. I say this because ..:
> 
>  - this bug isn't marked blocking- nor wanted-firefox3+
>  - QA hasn't been planning on testing this mode
>  - all of the reviewers will be focusing on blocking bugs first
>  - some of Kai's comments make me think that there might be more thinking to be
> done here
> 
> This is great stuff, and I think a fantastic and huge step towards a much
> desired ability to do true private browsing. I'm looking forward to playing
> with the tryserver builds, and am truly appreciative of all the work put into
> this, and by no means am suggesting we abandon things. Just that we make sure
> to not get disappointed if it misses.

So, if I can get all of the reviews in time, would this be possible to land this for Firefox 3?  (I'm most worried about the "QA hasn't been planning on testing this mode" part.)

Also, I guess this won't be something which could be expected to land in a 3.0.x release, right?  All this haste is because I know many users which would appreciate a lot if their browser provided private browsing mode capability.  I personally use a script to launch Firefox with a new profile (and the -no-remote command line param) and delete it after the browser exits, but I'm sick and tired of it!  :-)

> (That's why I was asking about the ability to get this done as an add-on,
> though I understand that might be difficult due to the nature of the changes -
> tryserver builds help!)

If there's some way of increasing the users testing the tryserver builds, please let me know.  I'm running one myself (of course!).

Anyway, would it be OK to dance the Target Milestone field to match the next milestone until the Firefox 3 release and then move it to Future if it misses?  ;-)

Comment 131

[(no longer active)]

(In reply to comment #127)
> I resubmitted the latest patch to the tryserver, and am playing with the
> current one for now ...

Should it be available at <https://build.mozilla.org/tryserver-builds/>?  (It's not currently.)

Comment 132

[(no longer active)]

(In reply to comment #129)
> (In reply to comment #122)
> > I think, using this approach, we will have to play "catch up" forever.
> 
> Not only us. What about an extension which saves urls to prefs (e.g. NoScript)
> or to their own data file (e.g. Adblock Plus and Tab Mix Plus)? Will somebody
> from MoCo go through AMO and make sure that that's taken care of for all
> extensions - or will private browsing only be really supported for
> extension-less profiles after all (just to be sure)?

Naive (and non-practical) answer: the extensions can be modified to use the browser:private-browsing notification.

More reasonable: warn the user about the privacy concerns of such extensions.  Should this warning be in the UI, or in the docs, or both?

Imaginative answer: would it be possible to make some changes to the core Mozilla I/O code, so that when the private browsing mode is turned on, the I/O subsystem (part of it which handles in-profile data files) creates a new access path to the profile data: a copy-on-write (COW) scheme, which copies the modified data to memory, and reads such data from memory later on, until the private mode is switched off, which would cause the in-memory copies of data to be discarded, and the I/O handles restored to make further I/O happen on disk like usual?

Open questions:
1) Is this possible at all, from an architectural point of view?  (Are there any high-level docs available on the I/O architecture in Mozilla, designed for someone who's not a guru in that field, i.e., me?)
2) What to do with APIs which can't make the above requirement?  (What happens when a file grows in private browsing mode, and some code seeks into the newly appended portion, for example?)
3) Would it be possible to implement this from a logical point of view?  (How would code react to gets its storage "swapped" in and out from underneath it?)
4) How much of the code which accesses the filesystem uses Mozilla's I/O facilities?  (For example, would this cover code which stores data in sqlite)?

Comment 133

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #131)
> (In reply to comment #127)
> > I resubmitted the latest patch to the tryserver, and am playing with the
> > current one for now ...
> 
> Should it be available at <https://build.mozilla.org/tryserver-builds/>?  (It's
> not currently.)

Didn't compile due to a bad checkin from Places, trying again now, should appear with the bug number and "Patch v1" in the build name.

Comment 134

[Mike Beltzner [:beltzner, not reading bugmail]]

Builds are here:

http://people.mozilla.org/~beltzner/private-browsing-builds/

Comment 135

[Shawn Wilsher :sdwilsh]

Comment on attachment 298757 [details] [diff] [review]
Patch (v1)

>+  mInPrivateBrowsing = PR_FALSE;
That should be in the constructor (which you'll have to add).

You also don't need to remove the observer.

Comment 136

[(no longer active)]

Attachment: Patch (v1.1)

Unbitrotted patch.  Addressed the issues in comment 135.  I also removed the RemoveObserver calls for places where observers were added using weak references.

Comment 137

[Simon Bünzli]

Comment on attachment 301098 [details] [diff] [review]
Patch (v1.1)

In case you really want to go through with the issue this way:

>Index: browser/components/sessionstore/src/nsSessionStore.js
>===================================================================
>@@ -184,16 +189,18 @@ SessionStoreService.prototype = {
>+
>+    this._inPrivateBrowsing = false;

This isn't needed as _inPrivateBrowsing is already initialized to false and we only get through here once (and even _if_ we got through here again, we'd surely not want to bail out of private browsing mode).

>-    if (this._sessionFile.exists()) {
>+    if (this._sessionFile && this._sessionFile.exists()) {

What's this change for? _sessionFile and _sessionFileBackup should always be initialized at this point.

>+    var backup = [];

You're using this as a hash, not an array, so please make it a {}. (In fact, using |for (... in ...)| is highly discouraged for arrays while it's OK for iterating over the keys of a hash.)

>+    for (ix in this._windows)
>+      backup[ix] = eval(this._windows[ix]._closedTabs.toSource());

Nits: ix is never declared. And please add a comment as to why you're doing |eval(....toSource())|.

>+    for (ix in this._closedTabsBackup) {
>+      if (this._windows[ix]) {

Nits: ix isn't declared. And you can drop the braces for the if clause (as in one or two further places).

r+=me for the SessionStore changes with these issues fixed.

Comment 138

[[e-mail]]

i'm in favor of having this be a notification rather than a pref, but some services are lazily inited (nsCookieService, nsPermissionManager, most likely others). for instance, |firefox about:blank| won't init the cookieservice, and if the user enters private browsing they'll miss that notification. any ideas? having the cookieservice call up |nsIPrivateBrowsingService::GetPrivateBrowsing()| on init kinda sucks :/

Comment 139

[Benjamin Smedberg]

Why does that suck? I don't think we're going to have any choice but have a global mode flag *somewhere* which indicates whether we are "right now" in private browsing mode. I'm happy to hang that on nsIXULRuntime or a new interface, whichever is easier. Then we have global observer topics for when the private-browsing-mode changes.

This bug is huge, by the way. Could we document the current set of design decisions in the wiki page? Especially the invariants we're trying to preserve (no website data to disk during private-browsing), and how that actually affects cookie/cache/etc.

Comment 140

[[e-mail]]

(In reply to comment #139)
> Why does that suck?

it's another interface dependency added to all these services - not especially bad, given that we're adding all this PB code anyway, it just means nsIPB needs to live early in the build process. (i.e. before netwerk.) if not, something more intuitive than nsIXULRuntime might be nice, <shrug>...

and agreed, some concrete documentation to refer back to here during discussion would be awesome.

Comment 141

[Glenn Randers-Pehrson]

Maybe this should be a separate bug/enhancement request, but it would be nice to be able to clear a particular item from one's "saved form" data without having to clear the entire history, for example when one inadvertently types a password into the login-name field.

Comment 142

[Glenn Randers-Pehrson]

Ah, thanks.  I had been trying BACKSPACE and nothing happened.

Comment 143

[Glenn Randers-Pehrson]

instead of using DELETE, that is.

Comment 144

[Benjamin Smedberg]

Comment on attachment 301098 [details] [diff] [review]
Patch (v1.1)

r- because this doesn't have tests and doesn't have a design doc

Comment 145

[alanjstr]

Should this be blocked by Bug 290456 ?

Comment 146

[(no longer active)]

(In reply to comment #145)
> Should this be blocked by Bug 290456 ?

I think it would be desirable to incorporate any solution developed for bug 290456 in Private Browsing, but I don't think it should block Private Browsing, especially because we don't currently handle clearing that sort of data right now...

Comment 147

[Mike Perry]

Hello, I'm the developer of the Torbutton Firefox extension (https://www.torproject.org/torbutton), and I've put a great deal of work and research (about 16 months now) into developing a 'private mode' via extensionland for Tor users. I approached the problem from the point of view that any amount of data that the browser leaks on to the disk or the network that is either uniquely identifying, can be used to build a random identifier, or indicative of their history is a vulnerability (See https://www.torproject.org/torbutton/design/#adversary and https://www.torproject.org/torbutton/design/#requirements).

While this was a different approach that seems to have been taken from this bug (which seems to have been attacked primarily from a usability perspective), it seems to have led us to similar conclusions.

I've documented everything I have learned and implemented in the Torbutton design document here: https://www.torproject.org/torbutton/design/

A couple of comments I can give from experience that weren't covered in your above discussion, or that I'd just like to re-emphasize:

0. Pages loaded in non-private mode can do all sorts of automatic network activity, set timers, attempt to re-authenticate, send unique identifiers stored in 'DOM Storage', etc etc during private mode.

1. Content window Javascript can access session history to navigate back and forward for the user to cause the local network (in our case the exit node) to learn a bit about their most recently accessed private or non-private session history.

2. Formfill and password saving can write private data to disk, and read it back where content window Javascript can inspect the values.

3. Livemarks refreshes currently cannot be disabled, and will still happen in the background during private mode (Bug 436250), exposing potentially private data to the local network (such as wikipedia pages you are an editor of, etc)

4. Plugins have their own cookies that can still be transmitted in private mode (see http://epic.org/privacy/cookies/flash.html).

5. Users hate it when a private mode does anything to permanently alter any of their non-private data. The only way to do this and have it ever be used is to isolate the non-private data so that it can't be touched during private mode, but is restored upon entrance back into non-private mode.

6. Custom SSL certificates are still available and can be probed for.

7. (Already covered briefly, would like to re-emphasize): History disclosure is a big big problem (see http://gemal.dk/browserspy/css.html and http://ha.ckers.org/weird/CSS-history.cgi http://www.mikeonads.com/2008/07/13/using-your-browser-url-history-estimate-gender/), 
but it must not be solved by damaging the user's current stored history, or even 
making it inaccessible.

8. Private mode should also be a pref in addition to an observer event, so that components and windows loaded after the transition can still determine the current mode.


I've also cataloged a list of Firefox bugs that make much of the above difficult under the current version of Firefox: https://www.torproject.org/torbutton/design/#FirefoxBugs

Comment 148

[(no longer active)]

A while back Beltzner told me that the drivers feel that the risk in the approach I've took in my patches on this bug is too high and that they're more willing to take a Places-based solution.  Are there any updates on this?  Or should I just pick up my slack here and work on it more?

Comment 149

[Alex Faaborg [:faaborg] (Firefox UX)]

>Are there any updates

I'll try to get an answer to you soon about this

Comment 150

[(no longer active)]

Ping?

Comment 151

[(no longer active)]

According to <https://wiki.mozilla.org/Firefox3.1/Features>, this seems to have been dropped from 3.1.  Anyway, it would be nice to know what the plans here are for this to get into the next release.

Comment 152

[Andreas Goetz]

Too bad- the only new feature I was really looking for :(

Comment 153

[Glenn Randers-Pehrson]

A Slashdot article today says Microsoft is applying for some patents on private browsing.

Comment 154

[Kai Engert (:KaiE:)]

(In reply to comment #153)
> A Slashdot article today says Microsoft is applying for some patents on private
> browsing.

Seems like there was some confusion, it appears it's only about a trademark for product names.

Comment 155

[Kai Engert (:KaiE:)]

Adding Wan-Teh to cc list, as a functionality that uses a memory-only filesystem would probably involve new functionality at the NSPR level.

Comment 156

[Alex Faaborg [:faaborg] (Firefox UX)]

>Ping?

Sorry about the lag, I thought mconnor had been in touch with you.  Recent development with Chrome will likely make finally getting private browsing mode shipped a priority for 3.1, but I think we are now targeting a more lightweight implementation.

What we need to figure out is if you should continue your implementation for use in a later release.

Here are some details mconnor sent me in email a few days ago about a possible strategy for 3.1:

>Main goals:
>
>Ensure that users can't be tracked when doing "private" things.  There
>should be a clear line drawn between your "public" and "private"
>browsing sessions.  It is acceptable to let things touch magnetic
>storage, as long as the cleanup mechanism is robust enough to clean
>up.
>
>It is also acceptable to retain data that users explicitly save
>(per-site permissions via prefs, bookmarks, etc)
>
>Non-goal for 3.1: Separate process sharing (some) data.  When we get
>process-per-tab we can make it more IE-like, but doing this also means
>that we have to have something like their "hey, you're in private
>browsing mode" banner on the URL bar for all the world to see.  Which,
>to me, is fail.
>
>
>Cookies:
>
>On entry:
>
>Write cookies to disk, drop the in-memory hashtable.
>
>During:
>
>All cookies are treated as session cookies.
>
>Exit:
>
>Drop the hashtable, reload from disk.
>
>
>History:
>
>On entry:
>
>Record timestamp of the last visit recorded.
>
>During:
>
>IsVisited always returns false (no link coloring spying)
>AddVisit silently fails.
>
>Exit:
>
>Ensure any visits recorded after the timestamp are purged (shouldn't
>be needed, but might be useful as a sanity check).
>
>
>Site Permissions:
>
>Page Info tab is disabled.
>Will not prevent users from explicitly adding exceptions via prefs.
>
>Passwords:
>
>Do not prompt to save passwords.
>Passwords will not autofill, but will be available for autocomplete.
>
>Other:
>
>Autocomplete will be available, but will not remember data entered.
>DOMStorage will not allow reading or writing of data (need JST/Enn
>feeedback on how to do this cleanly)
>All authenticated sessions will be logged out entering and leaving
>private mode.
>Downloads will be removed from dlmgr on completion.
>
>
>Optional: Save session and close all browser windows, and restore after
>exiting private mode?  Seems reasonable enough, especially if we can
>add the session store override to save SSL form data as a one-off...

Comment 157

[Jaquez]

I like the direction this is headed, especially if any of it shows up in 3.1 but I would like to suggest that IsVisited always returning false be rethought.

I find link coloring to be quite useful, particularly when going one by one through a list of links (e.g., on Google or a shopping site).

If it a matter of trying to figure out how to keep this off the disk and would require major re-work of that code, I can understand pushing it out until a later version but, to me, this is a major disadvantage of private mode: it's my biggest complaint about Stealther.

Comment 158

[alanjstr]

How about a second set of files for private browsing that automatically get scrubbed.  Alternatively, an indicator (or column) in the sqllite DB that marks something as from the private session.

Comment 159

[Emil Ivanov]

(In reply to comment #158)
> Alternatively, an indicator (or column) in the sqllite DB that marks
> something as from the private session.

This sounds me like session history, just like session cookies.

Comment 160

[Recall]

I think Distrust has the best implementation of private browsing. It has an icon at the bottom which you click to enter a session. Then when you have finished, you unclick it and your previous tabs are restored and no history is saved.

For cookies, they are treated as session cookies and deleted at shutdown.

https://addons.mozilla.org/en-US/firefox/addon/1559

Contact site for the maker

http://www.gness.com/distrust/

Comment 161

[(no longer active)]

Attachment: Patch (v2.0)

This patch implements the Private Browsing mode as described in comment 156.  All parts of the requirements are met here, except the DOM storage, which I'd be happy to have a feedback from JST or Enn on how best to tackle this in that module as neatly as possible.

Some parts of the code have not changed from the previous WIP patches, some other parts have been removed now that our requirements are a bit lighter, and some rewriting has also been performed.  It would be great if mconnor and other could test this both functionality and code-wise.  Also, I'd like to have beltzner's input on this as well, both on how the private browsing mode with the approach in comment 156 feels, and also on our chances to have this for 3.1.

It would also be great if someone with appropriate access could submit this patch to the try server for the users to try out actual builds.

Last, but not least, like with the previous patches, the current menu item is a mortal and mere hack which will be removed from the final version of this patch.  The UI should be decided and implemented in bug 411929.

Comment 162

[Neil Deakin]

(In reply to comment #161)
> Created an attachment (id=337327) [details]
> Patch (v2.0)
> 
> This patch implements the Private Browsing mode as described in comment 156. 
> All parts of the requirements are met here, except the DOM storage, which I'd
> be happy to have a feedback from JST or Enn on how best to tackle this in that
> module as neatly as possible.

It already supports session storage via permissions like cookies do. You should be able to either set that or modify nsDOMStorage::CanUseStorage as needed.

Dave Camp is the current owner of the code.

Comment 163

[(no longer active)]

(In reply to comment #162)
> It already supports session storage via permissions like cookies do. You should
> be able to either set that or modify nsDOMStorage::CanUseStorage as needed.
> 
> Dave Camp is the current owner of the code.

A quick look at CanUseStorage() and it seems to be only used in one place in nsDOMStorage implementation.  Is this enough to make this function return false during the private browsing session so that websites can't access or store data in this mode?

Comment 164

[Michael Ventnor]

Was cache handling dropped from the design? It seems like another thing we should look after in priv browsing.

Comment 165

[(no longer active)]

(In reply to comment #164)
> Was cache handling dropped from the design? It seems like another thing we
> should look after in priv browsing.

Yes, I think so too.  I had an implementation of cache handling in the previous patch which I deleted out.  I'm not sure about the rationale for dropping cache handling from the design though.  Mconnor, can you elaborate please?

Comment 166

[(no longer active)]

I prepared builds for Windows and Linux with the latest patch:

<http://ehsanakhgari.org/blog/2008-09-08/private-browsing-builds-ready>

Please take some time to test and evaluate it.  Also, if anyone owns a Mac and is willing to create a Mac build as well, that would be great!

Comment 167

[Johnathan Nightingale [:johnath]]

(In reply to comment #161)
> Created an attachment (id=337327) [details]
> Patch (v2.0)
...
> It would also be great if someone with appropriate access could submit this
> patch to the try server for the users to try out actual builds.

Just submitted to try server.  Assuming it builds cleanly, it should show up here in an hour or two:

https://build.mozilla.org/tryserver-builds/?C=M;O=D

Comment 168

[(no longer active)]

(In reply to comment #167)
> Just submitted to try server.  Assuming it builds cleanly, it should show up
> here in an hour or two:
> 
> https://build.mozilla.org/tryserver-builds/?C=M;O=D

<http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1220880196.1220880386.10390.gz>

Seems like a wrong -p option to patch.  patch should be invoked with -p1, I guess.  Identical results on Win32 and Mac.  Can you please re-submit the patch to try server?

Comment 169

[Dão Gottwald [::dao]]

I re-submitted the patch.

Comment 170

[Mike Connor [:mconnor]]

FWIW, spec I wrote is https://wiki.mozilla.org/User:Mconnor/PrivateBrowsing

If this patch is working to implement this spec, fantastic!  I'll take a look through tonight.  We can and will get this into 3.1 one way or another.

Comment 171

[(no longer active)]

(In reply to comment #170)
> FWIW, spec I wrote is https://wiki.mozilla.org/User:Mconnor/PrivateBrowsing

Thanks for the link.  For reference of those who want to test this patch without building it themselves, here is the link to the try server builds:

<https://build.mozilla.org/tryserver-builds/2008-09-08_11:[e-mail]-1220897935/>

> If this patch is working to implement this spec, fantastic!  I'll take a look
> through tonight.

Yes, it does.  Originally I based my work on comment 156, but this spec is somewhat different to that (IINM only with regard to the Permissions section).  I can easily remove the pageinfo-specific parts from this patch, which should bring this patch close to the spec you wrote.  The only remaining part is DOM Storage handling.  Enn provided some feedback on it (comment 162), and I'm waiting for more feedback from Dave Camp (comment 163).

Also, I think we should add cache handling to the spec, because sensitive data about user's private session could be stored on disk as part of the cache, and accessing that data isn't that hard (doesn't even require manual traversal of cache data files, about:cache comes to rescue!).

Please let me know what you think!

> We can and will get this into 3.1 one way or another.

That's very good to hear.  I'm ready to work on this full-speed, because I guess it requires a good amount of time for QA after landing.

Comment 172

[Dave Camp (:dcamp)]

(In reply to comment #163)
> (In reply to comment #162)
> A quick look at CanUseStorage() and it seems to be only used in one place in
> nsDOMStorage implementation.  Is this enough to make this function return false
> during the private browsing session so that websites can't access or store data
> in this mode?

Yeah, that should work fine.  Most entry points call CaheStoragePermissions(), which calls (and doesn't actually cache, apparently) CanUseStorage().

Comment 173

[(no longer active)]

Attachment: Patch (v2.1)

Changes in this patch:

 * The page info stuff was removed, as per the spec.
 * The passwordmgr code was changed because my previous patch made it dependent on the browser code which is not acceptable.  I also removed the PrivateBrowsingListener.jsm code entirely (which was a stupid idea of mine to begin with -- it can all be done using the Private Browsing service itself!).
 * The DOM Storage related code is now implemented.

One thing that I'm not sure and I need Dave's feedback on is whether the DOM Storage manager gets initialized at app startup or is it possible to be initialized lazily?  If the latter is the case, I need to add some code to detect at initialization time whether the private browsing mode is on or off, which may cause me to revive a trick I made in one of the previous WIPs (for supporting nsISupportsPRBool by the Private Browsing service, because dom code can't be made dependent on browser in which the nsIPrivateBrowsingService interface is defined).  It would be trivial, so I'm ready to provide a new patch if Dave confirms that it's possible for the DOM Storage manager to be initialized *after* the user has already entered the private browsing mode.

Like before, feedback and try server builds are welcome! :-)

One final note: I still think that we need cache handling here...

Comment 174

[(no longer active)]

It is important for extensions to be able to interact with the private browsing mode and capture its notifications.  I wrote a wiki page explaining the APIs for extensions and provided a number of code samples.  This can later be moved on to MDC.  You can see the page here: <https://wiki.mozilla.org/User:Ehsan/PrivateBrowsingForExtensions>

Obviously it's just a draft at this stage, and I'll keep on updating it as the API changes in this bug.

Comment 175

[Justin Dolske [:Dolske]]

(In reply to comment #173)

>  * The passwordmgr code

A few comments:

* Didn't an earlier version of this feature fire a notification when private browsing mode was entered or left? Making the pwmgr poll the service during pageload is kind of sucky.

* It would be cleaner to have the pwmgr not try to prompt for saving/changing a login in the first place, instead of having the code in nsLoginMangerPrompter.js silently bail out when it's attempted.

* The spec says form logins should have the autocomplete attached, but not autofilled. For HTTP auth, should the popup dialog be prefilled? Otherwise such saved logins are inaccessible. The user can click cancel to not authenticate with the site, although I can see an argument for that being too easy to accidentally forget. :)

Comment 176

[Dave Camp (:dcamp)]

(In reply to comment #173)
> One thing that I'm not sure and I need Dave's feedback on is whether the DOM
> Storage manager gets initialized at app startup or is it possible to be
> initialized lazily?  If the latter is the case, I need to add some code to
> detect at initialization time whether the private browsing mode is on or off,
> which may cause me to revive a trick I made in one of the previous WIPs (for
> supporting nsISupportsPRBool by the Private Browsing service, because dom code
> can't be made dependent on browser in which the nsIPrivateBrowsingService
> interface is defined).  It would be trivial, so I'm ready to provide a new
> patch if Dave confirms that it's possible for the DOM Storage manager to be
> initialized *after* the user has already entered the private browsing mode.

The storage manager is initialized at startup.

But if pieces below the browser are using this feature, wouldn't it make sense to put nsIPrivateBrowsingService.idl somewhere lower in the stack?  It doesn't seem like it's any cleaner to have everything built before browser/ either be initialized at startup or use a different interface to access the service as anything after browser/.

Comment 177

[Shawn Wilsher :sdwilsh]

Comment on attachment 337732 [details] [diff] [review]
Patch (v2.1)

So, the download manager may or may not be initialized at startup, so you can't depend on receiving the notification.

Secondly, this really needs some tests.  Toolkit has a requirement for any new feature to land with tests, and I think browser may be the same way.

You won't get my review for the download manager bits without a test to show that it works as expected.

Comment 178

[Johnathan Nightingale [:johnath]]

(In reply to comment #177)
> You won't get my review for the download manager bits without a test to show
> that it works as expected.

... but we really hope you will supply those tests, because the work so far is great, Ehsan.  Really - thank you for continuing to plug away at this.

Comment 179

[Justin Dolske [:Dolske]]

(In reply to comment #177)
> So, the download manager may or may not be initialized at startup, so you can't
> depend on receiving the notification.

The typical pattern would be for a component to get the current state when it starts up, at the same time it registers an observer.

Comment 180

[(no longer active)]

(In reply to comment #175)
> (In reply to comment #173)
> 
> >  * The passwordmgr code
> 
> A few comments:

Thanks for the notes! :-)

> * Didn't an earlier version of this feature fire a notification when private
> browsing mode was entered or left? Making the pwmgr poll the service during
> pageload is kind of sucky.

Yeah, it still does.  I'll change this in the next revision of the patch.

> * It would be cleaner to have the pwmgr not try to prompt for saving/changing a
> login in the first place, instead of having the code in
> nsLoginMangerPrompter.js silently bail out when it's attempted.

Agreed.  I'll try to move as much of the code as possible to nsLoginManager in
the next revision.

> * The spec says form logins should have the autocomplete attached, but not
> autofilled. For HTTP auth, should the popup dialog be prefilled? Otherwise such
> saved logins are inaccessible. The user can click cancel to not authenticate
> with the site, although I can see an argument for that being too easy to
> accidentally forget. :)

Well, that's really something for the spec to address.  Currently the patch
doesn't autofill form fields, and leaves HTTP auth dialogs blank.  I can easily
change this so that they get filled, provided that mconnor is in favor of this
change.

Comment 181

[(no longer active)]

(In reply to comment #176)
> But if pieces below the browser are using this feature, wouldn't it make sense
> to put nsIPrivateBrowsingService.idl somewhere lower in the stack?  It doesn't
> seem like it's any cleaner to have everything built before browser/ either be
> initialized at startup or use a different interface to access the service as
> anything after browser/.

I also think browser/ is not a good place to put nsIPrivateBrowsingService.idl, but I'm not sure where to put it.  Can you suggest some place?  Ideally we need some place where we can make all of the modules dependent on it, so that they can all access nsIPrivateBrowsingService, but I'm not sure if such a place exists in the tree...

Comment 182

[(no longer active)]

(In reply to comment #177)
> So, the download manager may or may not be initialized at startup, so you can't
> depend on receiving the notification.

Good point to mention.  Based on comment 179, I think I'm going to change the logic for each module to get the current status on startup (no matter if it's initialized at startup or not), and I'll make sure to handle download manager this way as well.

> Secondly, this really needs some tests.  Toolkit has a requirement for any new
> feature to land with tests, and I think browser may be the same way.
> 
> You won't get my review for the download manager bits without a test to show
> that it works as expected.

Sure.  The next thing I'll be looking into is getting started to write some tests for this code.  I'll try to make this in-litmus- and in-testsuite+.  :-)

(In reply to comment #178)
> ... but we really hope you will supply those tests, because the work so far is
> great, Ehsan.  Really - thank you for continuing to plug away at this.

Thanks!  :-)

(In reply to comment #179)
> The typical pattern would be for a component to get the current state when it
> starts up, at the same time it registers an observer.

I see.  I think before jumping at this, I'll need to find some place else to put nsIPrivateBrowsingService.idl in, so that all the modules can query the service on startup.  I need help finding the appropriate place, though.

If such a place cannot be found, I'm thinking of defining another notification, named something like "private-browsing-query" which passes a nsISupportsPRBool as the subject (like "quit-application-requested"), and make the private browsing service respond to this notification by passing the current status in aSubject.

Any hints on whether I'm barking at the right tree or not?

Comment 183

[Mike Connor [:mconnor]]

Ehsan, if it'd help I can write up a test plan for what we need to test.  If you want to work together on this, just find me on IRC.

(In reply to comment #157)
> I like the direction this is headed, especially if any of it shows up in 3.1
> but I would like to suggest that IsVisited always returning false be rethought.
> 
> I find link coloring to be quite useful, particularly when going one by one
> through a list of links (e.g., on Google or a shopping site).

Given that this can be (ab)used by sites, it doesn't make sense to expose your non-private history to them, and since we're not recording visits in private mode, there's nothing reasonable we can return.

Comment 184

[Sylvain Pasche]

I gave a try with the build linked in comment 171. I enabled private browsing, visited a given website, disabled it and grepped the profile for occurrences of that website URL. I could find references in the files:
profile/Cache/<a bunch of files>
profile/cookies.sqlite
profile/places.sqlite

(and in session_restore.js before I quit the browser). The cache and session restore are not handled yet, but shouldn't cookies and places not contain traces?

Comment 185

[(no longer active)]

(In reply to comment #184)
> The cache and session
> restore are not handled yet, but shouldn't cookies and places not contain
> traces?

Thanks for testing this, Sylvian.  Yes, you're right, cookies and places are supposed not to leave a trace.  Can you open the sqlite DBs and check in which table(s) the mentioned URL appears please?

Comment 186

[Sylvain Pasche]

sure, I accessed slashdot with private browsing mode enabled:

sqlite3 cookies.sqlite .dump|grep slashdot
INSERT INTO "moz_cookies" VALUES(1221074391930408,'__utma','9273847.3921509624962351000.1221074392.1221074392.1221074392.1','.slashdot.org','/',1284146393,1221074393924407,0,0);
INSERT INTO "moz_cookies" VALUES(1221074391930799,'__utmb','9273847.1.10.1221074392','.slashdot.org','/',1221076193,1221074393926321,0,0);
INSERT INTO "moz_cookies" VALUES(1221074391932333,'__utmz','9273847.1221074392.1.1.utmcsr=(direct)|utmccn=(direct)|utmcmd=(none)','.slashdot.org','/',1236842391,1221074391932333,0,0);
INSERT INTO "moz_cookies" VALUES(1221074392683439,'__qca','1221074392-53816111-35021676','.slashdot.org','/',2147385600,1221074392683439,0,0);


sqlite3 places.sqlite .dump|grep slashdot
INSERT INTO "moz_favicons" VALUES(8,'http://slashdot.org/favicon.ico',X'000001000100101010000000000028010000160000002800000010000000200000000100040000000000C0000000000000000000000000000000000000000000000080800000FFFFFF00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000111000001110000011100001111100000111000111110000011100011111000000111000111000000011100000000000000111000000000000011100000000000000111000000000000011100000000000000111000000000000011100000000000000000000000000000000000000FFFF0000E7E70000C3C30000C3810000E1810000E1830000F0C70000F0FF0000F87F0000F87F0000FC3F0000FC3F0000FE1F0000FE3F0000FFFF0000FFFF0000','image/x-icon',1221160794701793);
INSERT INTO "moz_places" VALUES(45,'http://www.google.com/search?ie=UTF-8&oe=UTF-8&sourceid=navclient&gfns=1&q=slashdot','search','moc.elgoog.www.',1,0,0,NULL,100);
INSERT INTO "moz_places" VALUES(46,'http://slashdot.org/','Slashdot: News for nerds, stuff that matters','gro.todhsals.',1,0,0,8,100);

Comment 187

[(no longer active)]

Thanks for the update, Sylvian.  The places part is half about the favicon service (which I didn't handle before, and should do so in the next revision).  The other half might be because of what I'm doing in AddVisit only makes sure nothing is recorded in moz_historyvisits, and moz_places would still be exposed to private data without further checks.

But I don't understand the cookies part at all.  Would you mind checking out to see if it's reproducible, both with the same profile and a new one, and post here a detailed STR here?  And also make sure you delete all this data before testing this again with the same profile (sorry for stating the obvious)?

Thanks!

Comment 188

[[e-mail]]

(In reply to comment #187)
> But I don't understand the cookies part at all.

at first glance, the problem is you set an observer in the cookieservice, but it's inited lazily - not until first pageload. if Sylvain enabled PB before loading a page, cookieservice won't catch that notification. you'll need to do what dolske suggested in comment 179: check PB mode in Init() and also register an observer.

Comment 189

[(no longer active)]

(In reply to comment #188)
> at first glance, the problem is you set an observer in the cookieservice, but
> it's inited lazily - not until first pageload. if Sylvain enabled PB before
> loading a page, cookieservice won't catch that notification. you'll need to do
> what dolske suggested in comment 179: check PB mode in Init() and also register
> an observer.

Thanks for the pointer, Dan!  Of course, you're right.  I'm going to work on this tomorrow.  If by that time someone doesn't come up with a solution to comment 181 (where to put nsIPrivateBrowsingService.idl), I'm going to take the "private-browsing-query" notification approach (comment 182)...

Comment 190

[Sylvain Pasche]

Here's the detailed steps:

Download and Extract the Firefox build somewhere
Create a new profile directory:
rm -rf /tmp/profile; mkdir /tmp/profile
Start it (adapt this for your OS)
./Minefield.app/Contents/MacOS/firefox -no-remote -profile /tmp/profile
Check Tools > Private Browsing
Open www.slashdot.org
Go back to Minefield Home page
Uncheck Tools > Private Browsing
Quit Firefox
Grep the profile:
grep -rli slashdot /tmp/profile
Expected: nothing
Actual: see comment 184


Minefield homepage (http://www.mozilla.org/projects/minefield/) does not set cookies, so Dan explanations apply in this situation.

Comment 191

[[e-mail]]

(In reply to comment #190)
> ./Minefield.app/Contents/MacOS/firefox -no-remote -profile /tmp/profile

> Minefield homepage (http://www.mozilla.org/projects/minefield/) does not set
> cookies, so Dan explanations apply in this situation.

if that command loads the homepage on startup, that'll pull in the cookieservice (regardless of whether it sets anything... it needs to check if there are cookies to send, too). my explanation only applies if it loads about:blank.

something else may be amiss, throw in some debug statements and fiddle around?

(In reply to comment #189)
> comment 181 (where to put nsIPrivateBrowsingService.idl), I'm going to take 

it'll need to be somewhere above the netwerk tier, so toolkit's out, no?

Comment 192

[Johnathan Nightingale [:johnath]]

I notice that neither the functional spec, the bug comments thus far, or the current patch seem to account for safebrowsing (anti-phishing/malware).

Presumably we still want to protect people in this mode from those attacks (indeed, they may be more likely to visit disreputable or compromised sites).  On the other hand, the pingback behaviour in the safebrowsing protocol means that when a reported malware/phishing site is hit, FF will go double-check, which means a hash request to google.  That is probably not behaviour that our users would expect, when in PB mode.

One reasonably straightforward technical solution there is to keep checking sites against the local DB, but not perform the pingbacks, while in PB mode.  Dave Camp (already cc'd) can comment on the technical feasibility there, but there is also a concern with that approach, namely that sites which have been removed from the list after the user enters PB mode will still be marked as dangerous.  We don't like to mark sites as dangerous that aren't actually.  Still, I'm not sure either of the other obvious solutions (disable malware/phishing protection entirely, or leave it entirely operational) are desirable.

I don't want to scope creep the bug - if we want to file a followup for this, we can, but I think we need an answer to it.  People using this mode are precisely the people who would be sensitive to automatic pingbacks.  (To be clear, these only occur for reported bad sites, not for every visit - a user might go their whole life without encountering a single one.  But then again, they may not. )

Comment 193

[Dave Camp (:dcamp)]

Blocking without a pingback would be feasible, but you'd end up with false positives in addition to stale blocks (not particularly often, but it could happen).

What might work is a separate error page for blocks that are unconfirmed due to private browsing mode.  It should be possible to add a "confirm this match" link that does the pingback manually.

Comment 194

[Radek 'sysKin' Czyz]

Is there any hope to block content-prefs.sqlite from "leaking" pages visited in private browsing mode? Simply changing zoom level pushes the page to this database and I remember there were some difficulties removing it, even with "clear private data".

Comment 195

[(no longer active)]

(In reply to comment #191)
> if that command loads the homepage on startup, that'll pull in the
> cookieservice (regardless of whether it sets anything... it needs to check if
> there are cookies to send, too). my explanation only applies if it loads
> about:blank.
> 
> something else may be amiss, throw in some debug statements and fiddle around?

I'll investigate this further.

> (In reply to comment #189)
> it'll need to be somewhere above the netwerk tier, so toolkit's out, no?

Yes.

Comment 196

[(no longer active)]

(In reply to comment #192)
> People using this mode are
> precisely the people who would be sensitive to automatic pingbacks.

Hmmm, I was under the impression that safebrowsing only sent a hash digest of a URL to the provider when its (shorter) hash matches one of those downloaded from the provider periodically, right?  So there should not be anything revealing the URL that the user has visited in this process, right?

(In reply to comment #193)
> What might work is a separate error page for blocks that are unconfirmed due to
> private browsing mode.  It should be possible to add a "confirm this match"
> link that does the pingback manually.

If there are privacy concerns here, I think we can go with this solution, but I fail to see the privacy concerns anyway...  Anyway ultimately it's the call of someone more familiar with the code.

Comment 197

[(no longer active)]

(In reply to comment #194)
> Is there any hope to block content-prefs.sqlite from "leaking" pages visited in
> private browsing mode? Simply changing zoom level pushes the page to this
> database and I remember there were some difficulties removing it, even with
> "clear private data".

An earlier revision of my patch handled content prefs as well.  Mconnor: was omitting the content prefs from the functional specs deliberate?

BTW, based on IRC talk with mconnor earlier, cache handling will be added to the patch, and a session store related change is also along the way.  The current plans are closing all of the windows/tabs when entering the private mode, and restoring them all after exiting the private mode, so that there is a clear separation between the private and non-private windows/tabs as far as the users are concerned.

Comment 198

[Johnathan Nightingale [:johnath]]

(In reply to comment #196)
> (In reply to comment #192)
> > People using this mode are
> > precisely the people who would be sensitive to automatic pingbacks.
> 
> Hmmm, I was under the impression that safebrowsing only sent a hash digest of a
> URL to the provider when its (shorter) hash matches one of those downloaded
> from the provider periodically, right?  So there should not be anything
> revealing the URL that the user has visited in this process, right?

Your description of the double-check is correct:

 - User attempts to load a page whose (32-bit) hash matches one in the DB
 - We ask for the full length hash for that entry, to eliminate the possibility of collisions
 - At no point is the raw URL transmitted.

The concern arises because the list provider has all the "real" urls, so the usual difficulties of reversing a hash don't really apply.  If my client sends a double-check request, an unscrupulous list provider could figure out which of the entries in their list it corresponded to at which point they would know either that I had visited that site, or that I had visited a site which hash-collides with it.

That is a pretty minor leakage, particularly given the strength of the privacy policy google has in place around its collection of data, but it is non-negligible.  Dave's idea is probably a good one to consider as well, but yes, I think I will file a bug dependent on this one to track it, rather than risk derailing the progress here.

Comment 199

[Johnathan Nightingale [:johnath]]

Bug 454792 opened to track the safebrowsing discussion.

Comment 200

[(no longer active)]

Attachment: Patch (v2.2)

What's new in this patch:

 * Added the "private-browsing-query" notification support to nsPrivateBrowsingService, and added correct initialization code to all the existing modules.  Order of initialization for modules is insignificant here: if a module initializes before the private browsing service, and sends the query notification, it will not be handled, so the module assumes that we're not in private browsing mode (correct assumption).  For all modules initialized after the private browsing service, the private browsing service will correctly respond to the query notification and the modules in question get initialized with the correct current private browsing status.
 * Handled Justin's first and second issues in comment 175.

Todo stuff for the next revision: add the cache (and content prefs service) handling, and improve the places handling of the private browsing mode (see comment 188).  Also unit tests are forthcoming, perhaps not in the next revision though (which I hope to get ready by tomorrow).

Comment 201

[(no longer active)]

Attachment: Patch (v2.3)

What's new in this version of the patch:

 * Handled the cache service with this logic: on entering the private mode, disable the disk and offline caches.  During the private mode, only the memory cache remains active.  When leaving the private mode, clear the memory cache, and enable the disk and offline caches, if they should be enabled based on their respective prefs.
 * Handled the content prefs service, so that things such as changing the zoom level on a site doesn't cause trails of the visit be left in the profile.
 * Made the favicon service aware of the private mode, and disable saving of favicons (and potentially addition of moz_places entries as Sylvian had observed in comment 186).
 * Made sure that the test laid out in comment 190 passes successfully.

This patch is worth having a try server build for, so if someone can submit it, that would be great!

I'll get started with some unit tests in the next revision.

Comment 202

[Dão Gottwald [::dao]]

(In reply to comment #201)
> Created an attachment (id=338299) [details]
> Patch (v2.3)

> This patch is worth having a try server build for, so if someone can submit it,
> that would be great!

https://build.mozilla.org/tryserver-builds/2008-09-12_06:[e-mail]-priv-20080912/

Note that Linux talos didn't like that build:
FAIL: Busted: tp
FAIL: browser frozen

Comment 203

[alanjstr]

I just want to confirm: This is not tab-independent, correct?  It applies to the whole browser.

Comment 204

[Yeik]

While you are working on this project, I had an idea for an improvement or addition to it.

Add a feature that will allow users to clear all items instantly for the sessions they had browsed during the session of mozilla. I doubt you keep up a history of each cookie and history of what that individual process/tab has done during the whole time it has been open (would be a memory hog). Sometimes a user is browsing and might have forgotten to set mozilla for private browsing instead allowing users to clear the sessions, even if it only pertains to what is currently open and associated cookies/history.

any feedback against this is welcome, just wanted to get it out in the open. may not be the right forum either.

Comment 205

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

I just took a quick look over it and it looks awesome. Great work!

One concern: From my quick look it didn't seem to be touching nsSessionStore.js. Are closed tabs remembered using your current code? (Again, quick look and I haven't tried it) I ask because I'm implementing "undo closed window" which is analogous to the "undo closed tabs" feature, and I'm trying to get a feel for how this might effect the time line for that, and vice versa.

Comment 206

[(no longer active)]

(In reply to comment #202)
> https://build.mozilla.org/tryserver-builds/2008-09-12_06:[e-mail]-priv-20080912/

Thanks!

> Note that Linux talos didn't like that build:
> FAIL: Busted: tp
> FAIL: browser frozen

Hmmm, that's weird, I can build and run the Linux version without any problems here.  Are there more detailed logs available?

(In reply to comment #203)
> I just want to confirm: This is not tab-independent, correct?  It applies to
> the whole browser.

Yes, that's correct.  The Private Browsing implementation is global to all of the Firefox windows.  According to the latest changes in the spec, the currently open windows and tabs will be closed when entering the private mode, so this may be a little less confusing.

(In reply to comment #204)
> Add a feature that will allow users to clear all items instantly for the
> sessions they had browsed during the session of mozilla. I doubt you keep up a
> history of each cookie and history of what that individual process/tab has done
> during the whole time it has been open (would be a memory hog). Sometimes a
> user is browsing and might have forgotten to set mozilla for private browsing
> instead allowing users to clear the sessions, even if it only pertains to what
> is currently open and associated cookies/history.

This is a good idea.  Currently the sanitizer (accessible from Tools > Clear Private Data) is able to clear the whole data, not the session specific data.  This is actually a sanitizer thing, and I'm not sure if it's not already filed as a bug.  Please search bugzilla and if it's not already filed, go ahead and file it, since this will need a separate bug report in order to work on.

(In reply to comment #205)
> I just took a quick look over it and it looks awesome. Great work!

Thanks! :-)

> One concern: From my quick look it didn't seem to be touching
> nsSessionStore.js. Are closed tabs remembered using your current code? (Again,
> quick look and I haven't tried it) I ask because I'm implementing "undo closed
> window" which is analogous to the "undo closed tabs" feature, and I'm trying to
> get a feel for how this might effect the time line for that, and vice versa.

According to the latest changes in the spec <https://wiki.mozilla.org/User:Mconnor/PrivateBrowsing#Session_Store>, session store will be handled in this way: when entering the private mode, all open windows and tabs will be closed, and only a single window will be left open.  The undo close tabs (and windows) list will be left untouched in private mode (i.e., the tabs and windows closed in private mode will not be added to them).  When leaving the private mode, all of the windows and tabs that the user had open before entering the private mode will be restored, and undo close tabs/windows list will get updated once again.

BTW, what's the bug# tracking the undo close windows work?  I'm personally interested in it.

Comment 207

[(no longer active)]

Attachment: Patch (v2.4)

This patch makes a small correction in the cache module handling to clear the memory cache when entering the private browsing mode (as per the spec).  Furthermore, it implements the session store handling as laid out in the spec.

For session store handling, nsSessionStore turns off writing the session data to disk while in private mode, and backs up the restore windows data when entering the private mode, and restores it after leaving it.  Also, it prompts the user if they want to close their current session and open a private session.  If the user says yes, then the current windows and tabs will be closed, and after leaving the private mode, they all will be restored.  If the user says no, their tabs and windows remain open and no restore takes place when leaving the private mode (the behavior of versions prior to 2.4).

This prompting is implemented in nsBrowserGlue, and can be overrided with the new browser.privatebrowsing.keep_current_session pref.  A mechanism is in place for extensions to override this UI (by handling the "private-browsing-start" notification, as I'll document in <https://wiki.mozilla.org/User:Ehsan/PrivateBrowsingForExtensions>), and the nsBrowserGlue behavior only kicks in when no extension handles this notification.  In case an extension provides its own UI here, the browser.privatebrowsing.keep_current_session will be ignored, and saving user's choice (if desired) will be the responsibility of the extension itself.

At this revision, the patch should be fully compatible with the spec.  Dao: can you submit this to the try server as well?  Thanks!

By the way, I've tested this on both Windows and Linux.  Let's see if this time Talos can run this successfully...

Comment 208

[Simon Bünzli]

Drive-by comments related to Session Restore:
* If you temporarily enter private browsing and have Firefox close your windows, what happens when you crash during private browsing? Will your original windows be restored? (AFAICT this should work already)
* When you quit while still in private browsing mode, what will be restored at the next startup with Session Restore switched on? (looks like this could be sensitive content, at least if you've decided not to close all non-private windows first)
* Instead of saving/restoring the list of closed tabs for all windows, I'd rather tag all tabs closed during private browsing mode and then just purge these when private browsing is over.

Comment 209

[Dão Gottwald [::dao]]

(In reply to comment #207)
> Created an attachment (id=338439) [details]
> Patch (v2.4)

https://build.mozilla.org/tryserver-builds/2008-09-13_07:[e-mail]-priv-20080913/

Comment 210

[(no longer active)]

(In reply to comment #208)
> Drive-by comments related to Session Restore:
> * If you temporarily enter private browsing and have Firefox close your
> windows, what happens when you crash during private browsing? Will your
> original windows be restored? (AFAICT this should work already)

Yes, the original windows will be restored.

> * When you quit while still in private browsing mode, what will be restored at
> the next startup with Session Restore switched on? (looks like this could be
> sensitive content, at least if you've decided not to close all non-private
> windows first)

Again, the original windows will be restored, even if you choose not to close all non-private windows when entering the private browsing mode.  Basically, no data about the session will be saved to the disk while in private browsing mode so the contents of sessionstore.js will not change while in private mode.

> * Instead of saving/restoring the list of closed tabs for all windows, I'd
> rather tag all tabs closed during private browsing mode and then just purge
> these when private browsing is over.

What about the case where the windows/tabs are closed when entering the private mode?  Will this work in that case as well?

Comment 211

[Simon Bünzli]

(In reply to comment #210)
> no data about the session will be saved to the disk while in private browsing
> mode so the contents of sessionstore.js will not change while in private mode.

You will however have to write to sessionstore.js during shutdown, otherwise Firefox will think it crashed at the next startup (unless you correctly safe the file before entering PB mode, which you currently don't). However AFAICT from skimming the code, you exit PB mode on quit-application-granted which will cause sessionstore.js to be written again with the state from when entering PB mode but (at least) updated cookies...

> What about the case where the windows/tabs are closed when entering the
> private mode?  Will this work in that case as well?

It does: You'll get the list(s) of recently closed tabs included in the browser state through getBrowserState and restore it through setBrowserState...

Comment 212

[(no longer active)]

(In reply to comment #211)
> You will however have to write to sessionstore.js during shutdown, otherwise
> Firefox will think it crashed at the next startup (unless you correctly safe
> the file before entering PB mode, which you currently don't). However AFAICT
> from skimming the code, you exit PB mode on quit-application-granted which will
> cause sessionstore.js to be written again with the state from when entering PB
> mode but (at least) updated cookies...

So, would that be enough for Firefox to detect that it was shut down cleanly?

> It does: You'll get the list(s) of recently closed tabs included in the browser
> state through getBrowserState and restore it through setBrowserState...

I see.  Thanks for clarification.  I'll try to get around to implement your suggestion soon.

Comment 213

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ehsan, shouldn't the menu item be checked when you have entered the "Private Browsing" mode? At least on OS X it cannot be seen and it makes hard to see in which state you are.

Comment 214

[(no longer active)]

(In reply to comment #213)
> Ehsan, shouldn't the menu item be checked when you have entered the "Private
> Browsing" mode? At least on OS X it cannot be seen and it makes hard to see in
> which state you are.

Yes, it should...  The thing is, I've been deferring all the UI work to bug 411929, but I guess we'd want this menu item at any rate (and if not, we can remove it in that bug) so I'll make a robust menu item which actually gets updated in response to changes in the private browsing mode in the next revision.  Thanks for bringing this into my attention, and sorry for the confusion it has caused in the QA work.

Comment 215

[(no longer active)]

Attachment: Patch (v2.5)

Just a small fix to the passwordmgr module, and improved the menu item in the Tools menu to fix the problem Henrik mentioned in comment 213.

Comment 216

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Try server builds for the latest patch v2.5 are available here:
https://build.mozilla.org/tryserver-builds/2008-09-14_14:[e-mail]-priv/

Comment 217

[Nochum Sossonko [:Natch]]

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1 ID:2008070208

Patch doesn't work for me on a fresh profile on Vista (at least the patched tryserver), I get these errors (and some others):

Warning: reference to undefined property Cc['@mozilla.org/browser/privatebrowsing;1']
Source File: chrome://browser/content/browser.js
Line: 8136

Error: Cc['@mozilla.org/browser/privatebrowsing;1'] is undefined
Source File: chrome://browser/content/browser.js
Line: 8136

Error: this._privateBrowsingService is null
Source File: chrome://browser/content/browser.js
Line: 8141

Error: uncaught exception: [Exception... "Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]"  nsresult: "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)"  location: "JS frame :: chrome://browser/content/browser.js :: anonymous :: line 987"  data: no]

Comment 218

[(no longer active)]

The latest try server build ID should be "Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080914141735 Minefield/3.1b1pre", but you're right that it doesn't work.  I inspected this a bit, and it seems like that the nsPrivateBrowsingService.js file does not exist in the components directory at all.  I took a look at the build log <http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1221426077.1221435503.1246.gz>, and the following statements seem to suggest that the component is correctly being copied to the components directory, but in fact, it's not.

for i in /e/builds/sendchange-slave/sendchange-win32-hg/mozilla/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js; do \
  dest=../../../../dist/bin/components/`basename $i`; \
  rm -f -f $dest; \
  /d/mozilla-build/python25/python /e/builds/sendchange-slave/sendchange-win32-hg/mozilla/config/Preprocessor.py -DOSTYPE=\"WINNT5.2\" -DOSARCH=WINNT -D_CRT_SECURE_NO_DEPRECATE=1 -D_CRT_NONSTDC_NO_DEPRECATE=1 -DWINVER=0x500 -D_WIN32_WINNT=0x500 -D_WIN32_IE=0x0500 -DX_DISPLAY_MISSING=1 -DMOZILLA_VERSION=\"1.9.1b1pre\" -DMOZILLA_VERSION_U=1.9.1b1pre -DHAVE_SNPRINTF=1 -D_WINDOWS=1 -D_WIN32=1 -DWIN32=1 -DXP_WIN=1 -DXP_WIN32=1 -DHW_THREADS=1 -DSTDC_HEADERS=1 -DWIN32_LEAN_AND_MEAN=1 -DNO_X11=1 -DHAVE_MMINTRIN_H=1 -DHAVE_OLEACC_IDL=1 -DHAVE_ATLBASE_H=1 -DHAVE_WPCAPI_H=1 -D_X86_=1 -DD_INO=d_ino -DMOZ_EMBEDDING_LEVEL_DEFAULT=1 -DMOZ_EMBEDDING_LEVEL_BASIC=1 -DMOZ_EMBEDDING_LEVEL_MINIMAL=1 -DMOZ_PHOENIX=1 -DMOZ_BUILD_APP=browser -DMOZ_XUL_APP=1 -DMOZ_DEFAULT_TOOLKIT=\"cairo-windows\" -DMOZ_DISTRIBUTION_ID=\"org.mozilla\" -DOJI=1 -DIBMBIDI=1 -DMOZ_VIEW_SOURCE=1 -DACCESSIBILITY=1 -DMOZ_XPINSTALL=1 -DMOZ_JSLOADER=1 -DNS_PRINTING=1 -DNS_PRINT_PREVIEW=1 -DMOZ_NO_XPCOM_OBSOLETE=1 -DMOZ_OGG=1 -DMOZ_MEDIA=1 -DMOZ_XTF=1 -DMOZ_CRASHREPORTER=1 -DMOZ_CRASHREPORTER_ENABLE_PERCENT=100 -DMOZ_MATHML=1 -DMOZ_ENABLE_CANVAS=1 -DMOZ_SVG=1 -DMOZ_UPDATE_CHANNEL=default -DMOZ_PLACES=1 -DMOZ_FEEDS=1 -DMOZ_STORAGE=1 -DMOZ_SAFE_BROWSING=1 -DMOZ_URL_CLASSIFIER=1 -DMOZ_LOGGING=1 -DMOZ_USER_DIR=\"Mozilla\" -DMOZ_ENABLE_LIBXUL=1 -DMOZ_TREE_CAIRO=1 -DHAVE_UINT64_T=1 -DMOZ_XUL=1 -DMOZ_PROFILELOCKING=1 -DMOZ_RDF=1 -DMOZ_MORKREADER=1 -DMOZ_DLL_SUFFIX=\".dll\" -DJS_THREADSAFE=1  -DNDEBUG -DTRIMMED $i > $dest; \
done


The build works correctly locally on at least Windows and Linux, and I'm not sure what I'm doing wrong which causes the try server builds not to include this component.  Can anyone see what is going wrong here?

Comment 219

[Mardeg]

A suggestion from another bug where patches were only working locally:
"are you creating new interfaces? If so, did you add the .xpt files to
the packaging manifests? If not, then that could be why things fail on packaged
builds but not your local builds."

Comment 220

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The only error I can see on OS X while toggling between the states is following entry:

Error: key is null
Source File: file:///Volumes/Minefield/Minefield.app/Contents/MacOS/components/nsUrlClassifierLib.js
Line: 1173

Which is:

PROT_UrlCryptoKeyManager.prototype.unUrlSafe = function(key)
{
--> return key.replace("-", "+").replace("_", "/");
}


On Windows XP I also cannot see the given errors and don't have Vista by hand for further tests until Thursday. But what I've recognized is that there is no dialog when entering the private browsing mode. I don't think that it is expected.

Further one more question: Why the private browsing mode is not sticky? After a restart it's still unset again. I believe in environments where this mode will be used at any time (e.g. internet cafe's) it will be a hassle to have to manually switch into this mode again and again.

Comment 221

[(no longer active)]

(In reply to comment #219)
> A suggestion from another bug where patches were only working locally:
> "are you creating new interfaces? If so, did you add the .xpt files to
> the packaging manifests? If not, then that could be why things fail on packaged
> builds but not your local builds."

Hmm, I am adding a new interface, and I think this might be the culprit.  Thanks for your suggestion.  I'll post an updated patch shortly.

Comment 222

[(no longer active)]

(In reply to comment #220)
> The only error I can see on OS X while toggling between the states is following
> entry:
> 
> Error: key is null
> Source File:
> file:///Volumes/Minefield/Minefield.app/Contents/MacOS/components/nsUrlClassifierLib.js
> Line: 1173
> 
> Which is:
> 
> PROT_UrlCryptoKeyManager.prototype.unUrlSafe = function(key)
> {
> --> return key.replace("-", "+").replace("_", "/");
> }

I get this as well, and I haven't looked into it.  Looks like somehow this component responds to the notifications it should ignore.  Anyway this doesn't prevent the patch from functioning.

> On Windows XP I also cannot see the given errors and don't have Vista by hand
> for further tests until Thursday. But what I've recognized is that there is no
> dialog when entering the private browsing mode. I don't think that it is
> expected.

Yes, on XP no error is logged to the console, but the component and the interface are not registered (which can be verified by examining Components.classes/Components.interfaces in the error console).

> Further one more question: Why the private browsing mode is not sticky? After a
> restart it's still unset again. I believe in environments where this mode will
> be used at any time (e.g. internet cafe's) it will be a hassle to have to
> manually switch into this mode again and again.

I'm not sure if we want to preserve this across sessions (it makes it easy for someone to know if you've been in private mode last time you closed Firefox or not) but I think we can add a pref to initiate the private mode each time Firefox starts automatically.

Comment 223

[(no longer active)]

Attachment: Patch (v2.6)

This is the same as 2.5, only with the xpt file being added to the installer packaging manifests.  I'm not sure if this causes nsPrivateBrowsingService.js to appear in the components folder in try server builds, though.  Can someone please submit this to try server?

Comment 224

[(no longer active)]

Attachment: Patch (v2.6)

Oops, please ignore the previous patch; I forgot to "hg add" the new files...  This one should be correct.

Comment 225

[Juha Laakso]

Is there any plans to handle Flash shared objects at this point or will it be
implemented afterwards? Does
https://bugzilla.mozilla.org/show_bug.cgi?id=290456 cover this or should a
separate bug be filled for private browsing mode case?

In any case, I see two possible solutions: 
1) Clear shared objects afterwards when user leaves the private browsing mode
2) Change Flash Player security settings to temporarily block shared objects. I
think these can be adjusted by setting variables in Flash
Player\macromedia.com\support\flashplayer\sys\settings.sol
Option 2 would honor the "Don't write to disk" principle.

Comment 226

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ehsan, the try server build failed. Looks like your patch is bitrotted:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1221476657.1221476875.32211.gz

Comment 227

[(no longer active)]

(In reply to comment #225)
> Is there any plans to handle Flash shared objects at this point or will it be
> implemented afterwards? Does
> https://bugzilla.mozilla.org/show_bug.cgi?id=290456 cover this or should a
> separate bug be filled for private browsing mode case?

I think it would be better to file a new bug, since that bug covers the sanitizer module.

> In any case, I see two possible solutions: 
> 1) Clear shared objects afterwards when user leaves the private browsing mode
> 2) Change Flash Player security settings to temporarily block shared objects. I
> think these can be adjusted by setting variables in Flash
> Player\macromedia.com\support\flashplayer\sys\settings.sol
> Option 2 would honor the "Don't write to disk" principle.

Are there documents from Adobe available that show how to do the second option in a cross-platform manner?

Comment 228

[(no longer active)]

Attachment: Patch (v2.6)

(In reply to comment #226)
> Ehsan, the try server build failed. Looks like your patch is bitrotted:
> http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1221476657.1221476875.32211.gz

Here's the unbitrotted patch diffed against the latest pulled hg trunk.  Can you submit it again?  Thanks! :-)

Comment 229

[:Gavin Sharp [email: [e-mail]]]

The latest patch doesn't contain the packages-static changes. You'll need to add nsPrivateBrowsingService.js to packages-static as well (see other JS components listed in that file).

Comment 230

[(no longer active)]

Comment on attachment 338622 [details] [diff] [review]
Patch (v2.6)

(In reply to comment #229)
> The latest patch doesn't contain the packages-static changes. You'll need to
> add nsPrivateBrowsingService.js to packages-static as well (see other JS
> components listed in that file).

You're right.  New patch forthcoming.  Hope I get it right this time... :-/

Comment 231

[(no longer active)]

Attachment: Patch (v2.6)

OK, this should be the right patch... ;-)

Comment 232

[Nochum Sossonko [:Natch]]

Till this gets to the tryserver... I took the liberty of uploading my own builds with this patch applied (builds are available for Windows only, sorry no cross-compiler;):

firefox.zip: https://www.yousendit.com/download/bVlCcHBLUEM1R05MWEE9PQ
.exe: http://www.yousendit.com/download/bVlCcHBFNXZUME9Ga1E9PQ

Only 100 downloads though so get to it! Great new feature, big fan of the work being done here, thanks alot.

Comment 233

[Marco Bonardo [:mak]]

i think that this could get better advantages of the changes we are experimenting in Places for fsync stuff, we will already have a memory table to hold places and history, and you could move using them.

Your patch has probably some issue on Places as it is, i did not read it all but if i read it correctly you don't allow adding places into moz_places returning always false to canAddURI. but then you save visits in a new table (that should be a TEMP one i think), but those visits will be orphans without a place. Also you're not ensuring to not add datas to annotations and inputhistory tables.
So you're practically removing places during private browsing, and that could make awesomebar/bookmarks and so on unusable while using it (usable only on old entries). Instead they should be correct until you're in private browsing, then clear all private data on exiting private mode.
Also what happens if a user wants to add a bookmark in private mode? probably we should still hold it since he explicitely asked to remember that site.

I have some ideas about this, so feel free to nag me on IRC if you want, sorry if i did not catch everything about your changes

Comment 234

[Nochum Sossonko [:Natch]]

Some notes:
With private browsing enabled I get the following error: Error: key is null
Source File: file:///C:/Users/Natch/Documents/mozilla_trunk/mozilla-central__release/dist/bin/components/nsUrlClassifierLib.js
Line: 1173
Also right-click bookmark a link is broken in private mode, the panel shows up on the left. I've seen other errors related to places stuff, though I haven't repro'd them reliably yet...

Comment 235

[(no longer active)]

(In reply to comment #234)
> Some notes:
> With private browsing enabled I get the following error: Error: key is null
> Source File:
> file:///C:/Users/Natch/Documents/mozilla_trunk/mozilla-central__release/dist/bin/components/nsUrlClassifierLib.js
> Line: 1173

I filed this as bug 455454, and attached a patch there to solve that problem.

> Also right-click bookmark a link is broken in private mode, the panel shows up
> on the left. I've seen other errors related to places stuff, though I haven't
> repro'd them reliably yet...

Is the placement of the panel the problem you're mentioning?  If so, that's where it appears normally, I think.  I guess Marco's comments (comment 233) might explain a bit of the problems you might have been experiencing in the private mode with Places.

I'll ping him on IRC for more info; in-memory tables look really interesting!

Comment 236

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #231)
> Created an attachment (id=338639) [details]
> Patch (v2.6)
> 
> OK, this should be the right patch... ;-)

And here the appropriate try server builds:
https://build.mozilla.org/tryserver-builds/2008-09-15_15:[e-mail]-bug248970/

Comment 237

[John P Baker]

For Content Preferences this doesn't follow the Wiki as it won't update an existing pref.

> Site Specific Prefs
>
> * Nothing special for enter/exit
> * During, we will not remember new entries, but will update existing ones. In current known usage (only used for zoom in Firefox) this is not a data leak. It is possible for extension uses to leak some data here, but there's a lot of things extension authors can do wrong.
> o This effectively means that setPref() will fail if hasPref() is false. This will likely be a silent failure so as to not throw for existing callers. 

Presumably changing to something like

>    setPref: function ContentPrefService_setPref(aURI, aName, aValue) {
>      // If the pref is already set to the value, there's nothing more to do.
> +    // If we are in private browsing mode, refuse to set the pref
>      var currentValue = this.getPref(aURI, aName);
> -    if (typeof currentValue != "undefined" && currentValue == aValue)
> +    if (typeof currentValue != "undefined" ? currentValue == aValue : this._inPrivateBrowsing)
>        return;

[possibly something more readable though!]

Comment 238

[(no longer active)]

Attachment: Patch (v2.6.1)

(In reply to comment #237)
> For Content Preferences this doesn't follow the Wiki as it won't update an
> existing pref.

This was changed after I implemented the content prefs part.  This new patch corrects the implementation in a way similar to what you suggested.

Comment 239

[Jim Jeffery not reading bug-mail 1/2/11]

Probably not related but thought I'd report with the latest build from the tryserver I am getting browser crashes when I visit www.neowin.net with the tracemonkey JIT content 'enabled'.  

The official nightly does not crash.  Sorry, breakpad did not kick in either. (but that's a known, ongoing issue with flash player)

Comment 240

[(no longer active)]

(In reply to comment #239)
> Probably not related but thought I'd report with the latest build from the
> tryserver I am getting browser crashes when I visit www.neowin.net with the
> tracemonkey JIT content 'enabled'.  
> 
> The official nightly does not crash.  Sorry, breakpad did not kick in either.
> (but that's a known, ongoing issue with flash player)

What platform are you on?  Maybe it's something changed after the latest nightly was built, can you check an hourly?  I'm not sure what may cause the crash here, and I can't reproduce it here.  Can others try this as well?

Comment 241

[Jim Jeffery not reading bug-mail 1/2/11]

I'm on Win32 Vista HP SP1 using today's official nightly:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b1pre) Gecko/20080916043910 Minefield/3.1b1pre Firefox/3.0 ID:20080916043910

Are there nightly's on the tryserver ?  
I was using this build when I was crashing:
https://build.mozilla.org/tryserver-builds/2008-09-15_15:[e-mail]-bug248970/

Comment 242

[(no longer active)]

(In reply to comment #241)
> I'm on Win32 Vista HP SP1 using today's official nightly:
> 
> Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b1pre)
> Gecko/20080916043910 Minefield/3.1b1pre Firefox/3.0 ID:20080916043910
> 
> Are there nightly's on the tryserver ?  
> I was using this build when I was crashing:
> https://build.mozilla.org/tryserver-builds/2008-09-15_15:[e-mail]-bug248970/

I don't get a crash (or anything unusual) when visiting neowin's home page neither in private mode or usual mode on XP SP2.  I have flashplayer 9.0.124.0.  Can you try an hourly?  What about the previous try server builds on this bug?

Comment 243

[Jim Jeffery not reading bug-mail 1/2/11]

I can try an hourly build when the next one comes out, about 20-30 mins.  

The build I noted above, was the first build I saw on the tryserver where Private mode actually worked, previous builds did not seem to enter Private-mode.  

I was crashing on NeoWin whether I was in Private mode or Normal mode.  

I was just wondering if part of the JIT/Tracemonkey stuff was maybe not in the build on the tryserver test builds ?  

If there are hourly builds for this bug could you point me to one?  I thought they were all hand-made...

Comment 244

[Jim Jeffery not reading bug-mail 1/2/11]

No crash on NeoWin with latest 'official hourly' from here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/1221568388/

This build or course does not have the InPriv patch -

Comment 245

[Jim Jeffery not reading bug-mail 1/2/11]

Using test build of InPriv:
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b1pre) Gecko/20080915154629 Minefield/3.1b1pre Firefox/3.0 ID:20080915154629

Bookmarking a page while in Private mode saves the bookmark.  I can see the bookmark I added after 'exit' from Private mode.

Comment 246

[(no longer active)]

(In reply to comment #245)
> Bookmarking a page while in Private mode saves the bookmark.  I can see the
> bookmark I added after 'exit' from Private mode.

This is the expected behavior: the bookmark should be saved, but no visit to the bookmarked site should be recorded.  See <https://wiki.mozilla.org/Firefox3.1/PrivateBrowsing/FunctionalSpec> for more info.

Comment 247

[(no longer active)]

(In reply to comment #243)
> I can try an hourly build when the next one comes out, about 20-30 mins.  
> 
> The build I noted above, was the first build I saw on the tryserver where
> Private mode actually worked, previous builds did not seem to enter
> Private-mode.  

Anyway, since you mention you got crashes both in private and normal mode, it makes sense to look into previous builds, I think.  If the crashes stop at a particular build, we can assume it's something I did between those revisions of the patch, and tracking it would be far simpler.

FWIW, I tested this both using Henrik's build, and my local build on Vista, neither crashed NeoWin...

> I was just wondering if part of the JIT/Tracemonkey stuff was maybe not in the
> build on the tryserver test builds ?  

I don't think so.  And there's nothing I do in the patch which touches any Tracemonkey stuff either.

> If there are hourly builds for this bug could you point me to one?  I thought
> they were all hand-made...

No, I meant the trunk hourly builds.  Sorry for the confusion.

Comment 248

[Marco Bonardo [:mak]]

(In reply to comment #246)
> This is the expected behavior: the bookmark should be saved, but no visit to
> the bookmarked site should be recorded.

interesting, you don't have a place and canAddURI return false (so you should not be able to add one), but using insertBookmark canAddURI is not checked, and a new place is added. I guess if this was done by design or we miss a check in getUrlIdFor.

Comment 249

[Jim Jeffery not reading bug-mail 1/2/11]

I don't know what else to do, I don't crash with the 'official' builds.  I guess we will see what happens when it finally makes it into the M-C official builds.

Comment 250

[Jim Jeffery not reading bug-mail 1/2/11]

When entering Private Mode the cursor is not focused in the LocationBar.  I would think this would be nice, as in normal op's when opening a 'blank' window the cursor is focued there.

Comment 251

[(no longer active)]

(In reply to comment #248)
> interesting, you don't have a place and canAddURI return false (so you should
> not be able to add one), but using insertBookmark canAddURI is not checked, and
> a new place is added. I guess if this was done by design or we miss a check in
> getUrlIdFor.

I carefully made sure that getUrlIdFor is not affected by the private browsing mode.  As far as I know, there are three places where getUrlIdFor is called with auto create mode turned on:

 * when creating bookmarks (which we should allow in private mode)
 * when creating favicons (which we shouldn't allow in private mode, and my patch handles it now)
 * when creating annotations.

I'm not really sure about what Places annotations mean, and where (in the UI) they are used, so I was not sure whether I need to turn off creating them in the private mode.  Any hints on that would be greatly appreciated.

Comment 252

[(no longer active)]

(In reply to comment #249)
> I don't know what else to do, I don't crash with the 'official' builds.  I
> guess we will see what happens when it finally makes it into the M-C official
> builds.

Are you using a new profile or an already existing one?  If the latter, can you try with a clean profile?

Also, can you disable your plugins one by one and see if the crashes happen with all of them disabled?

(In reply to comment #250)
> When entering Private Mode the cursor is not focused in the LocationBar.  I
> would think this would be nice, as in normal op's when opening a 'blank' window
> the cursor is focued there.

Hmmm, I think this should be handled by browser.js automatically when a new window is opened, but I may be wrong.  I'll need to check this, but at least I can confirm this problem here as well.  :-)

Comment 253

[Jim Jeffery not reading bug-mail 1/2/11]

I've been testing with an existing profile.  Just created a new one, no addons, no bookmarks.  Still crashes, and continues to crash when loading www.neowin.net even with all the plugins 'disabled'. 

JIT content is not enabled by 'default' yet.. are you sure you have it set to 'true' ? 

about:config
javascript.options.jit.content Value=true

I won't have time for anymore testing today..will catch up tomorrow.

Comment 254

[(no longer active)]

(In reply to comment #253)
> I've been testing with an existing profile.  Just created a new one, no addons,
> no bookmarks.  Still crashes, and continues to crash when loading
> www.neowin.net even with all the plugins 'disabled'. 
> 
> JIT content is not enabled by 'default' yet.. are you sure you have it set to
> 'true' ? 

Yes.

I'm CCing some TraceMonkey gurus to see if they can help (read comment 239 onwards).

Comment 255

[Ria Klaassen (not reading all bugmail)]

I get the same crash on www.neowin.net (Windows XP).

Comment 256

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Jim and Ria, could one of you try to get the stack trace with Windbg?

http://developer.mozilla.org/en/How_to_get_a_stacktrace_with_WinDbg

Comment 257

[(no longer active)]

Attachment: Patch (v2.7)

This test fixes a problem with half-renaming a pref, and adds a unit test for cookies, according to <https://wiki.mozilla.org/Firefox3.1/PrivateBrowsing/TestPlan#Unit_Tests>.

One thing I ran into problems with was running the nsICookieManager2.cookieExists method through the test.  cookieExists causes xpcshell to crash.  I'm not sure, but I think the crash may be because of the fact that the C++ implementation of cookieExists actually expects an nsCookie parameter, but since this method is not marked as noscript, I suspected that there may be something I'm missing.  Anyone can shed some light into this?

I had to disable the is_cookie_available2 checks for now because of the crash.

Comment 258

[Shawn Wilsher :sdwilsh]

(In reply to comment #257)
> One thing I ran into problems with was running the
> nsICookieManager2.cookieExists method through the test.  cookieExists causes
> xpcshell to crash.  I'm not sure, but I think the crash may be because of the
> fact that the C++ implementation of cookieExists actually expects an nsCookie
> parameter, but since this method is not marked as noscript, I suspected that
> there may be something I'm missing.  Anyone can shed some light into this?
That would indeed crash.  Can you file a bug on that please - the cookie service shouldn't assume it's an nsCookie.

Comment 259

[(no longer active)]

(In reply to comment #258)
> That would indeed crash.  Can you file a bug on that please - the cookie
> service shouldn't assume it's an nsCookie.

Done: bug 455598.  I attached a patch on that bug to fix this issue.  With that patch applied, the is_cookie_available2 checks in my test can be enabled, and the test correctly passes.

Comment 260

[Jim Jeffery not reading bug-mail 1/2/11]

(In reply to comment #256)
> Jim and Ria, could one of you try to get the stack trace with Windbg?
> 
> http://developer.mozilla.org/en/How_to_get_a_stacktrace_with_WinDbg

Wouldn't you know it, its not crashing today...if it comes up again, I'll try and get a trace with WinDbg.

Comment 261

[(no longer active)]

Attachment: Patch (v2.8)

What's new:

 * Now that the fix for bug 455598 landed, I enabled the nsICookieManager2.cookieExists tests in the cookies unit test.  Now, the cookie test follows the test plan exactly.
 * The previous patch had an oversight in the cookies unit test which caused it not to test the private browsing mode at all (duh!); fixed in this patch.
 * Added the unit test for content prefs, according to the test plan.
 * Added the browser UI test for passwordmgr, according to the test plan.
 * I tried to address the latest changes regarding passwordmgr in the functional spec as well (to enable filling of the HTTP auth dialog) and I noticed a strange problem:

If, while initiating the private browsing mode, I choose to keep the current session open, everything's cool.  If, however, I choose to save and close the session, the auth dialog turns up empty.  After enabling signon.debug, I noticed a couple of these messages which indicated that the username and password pair could not be decrypted:

PwMgr mozStorage: Failed to decrypt string: MDIEEPgAAAAAAAAAAAAAAAAAAAEwFAYIKoZIhvcNAwcECKifd14qd4yXBAhGB3/m2HDvtA== (NS_ERROR_FAILURE)

I tried running the below code in the error console, and it also failed with NS_ERROR_FAILURE:

Components.classes["@mozilla.org/security/sdr;1"].
getService(Components.interfaces.nsISecretDecoderRing).
decryptString("MDIEEPgAAAAAAAAAAAAAAAAAAAEwFAYIKoZIhvcNAwcECKifd14qd4yXBAhGB3/m2HDvtA==")

I also tried opening the password manager, and it was empty as well.  I'm not sure how saving and closing the session could affect the SecretDecoderRing service.  Does anyone have any ideas?

Comment 262

[Justin Dolske [:Dolske]]

You're most likely running into bug 437904. I'll try to investigate what's going on there; shouldn't be related to anything you're doing.

Comment 263

[(no longer active)]

(In reply to comment #262)
> You're most likely running into bug 437904. I'll try to investigate what's
> going on there; shouldn't be related to anything you're doing.

Looks related (I'm clearing the authenticated sessions upon entering the private browsing mode) but in this case, whether or not the user decides to save and close her current session affects the results...

I'd appreciate if you can take a deeper look and see if you can find something which my eyes have missed.

Comment 264

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #262)
> You're most likely running into bug 437904. I'll try to investigate what's
> going on there; shouldn't be related to anything you're doing.

That's true. Even without the patch running the above command in the Error Console throws this exception.

Ehsan, I've started a new try server build. Please have a look at the tinderbox site when it will be finished. I'm offline now.

http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTry

Comment 265

[(no longer active)]

(In reply to comment #264)
> That's true. Even without the patch running the above command in the Error
> Console throws this exception.

No, that doesn't happen here.  I guess the exception you're observing is caused by the fact that the encrypted string can't be decrypted with the key on your machine.  You can obtain an encrypted string for use in that code by looking into the passwords sqlite db, I guess (or reproducing the failure I observed with signon.debug turned on).

> Ehsan, I've started a new try server build. Please have a look at the tinderbox
> site when it will be finished. I'm offline now.
> 
> http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTry

Thanks, I'll try to keep an eye on it if I'm awake by the time it finishes (it's 2:30AM here), or check it tomorrow morning.  :-)

Comment 266

[Henrik Skupin [:whimboo][⌚️UTC+1]]

New try server build is available here:
https://build.mozilla.org/tryserver-builds/2008-09-17_14:[e-mail]-bug248970v2.8/

Comment 267

[(no longer active)]

Linux build was unsuccessful, but I doubt it had anything to do with the patch <http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1221688098.1221688924.20353.gz>.  Windows talos was orange with the error "previous cycle still running", and I'm not sure what that means (seems like many other patches caused an orange win32 talos with the same error message) <http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1221696517.1221696581.9936.gz>.

Comment 268

[Marco Bonardo [:mak]]

(In reply to comment #251)
> I'm not really sure about what Places annotations mean, and where (in the UI)
> they are used, so I was not sure whether I need to turn off creating them in
> the private mode.  Any hints on that would be greatly appreciated.

Annotations are notes that can be added by us or an extension developer to urls or bookmarks, so it can contain any data, personal or uri related data too (it depends on how the implementer uses it).
The main problem here is with page annotations, mainly because if an extension adds a page annotation during private browsing and the place_id does not exist we would probably try to add anno for place_id = -1 since you don't have a place.
You should probably early return in nsAnnotationService::SetPageAnnotation while in private browsing mode for now, or check for place id validity later (better handling could be done though).

about moz_inputhistory you should early return in nsNavHistory::AutoCompleteFeedback se we will not save what urls user has choosen in the awesomebar after searching for a certain text.

Comment 269

[Nochum Sossonko [:Natch]]

A few notes:
Download Manager is broken in private browsing mode, although files to get downloaded the DM cannot (and perhaps should not, but it should be notified of PB) tell you the progress of the download, also I stumbled across these errors:
Error: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsITransfer.onProgressChange64]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://global/content/contentAreaUtils.js :: anonymous :: line 138"  data: no]
Source File: chrome://global/content/contentAreaUtils.js
Line: 138
When completing the download:
Error: download is null
Source File: chrome://mozapps/content/downloads/DownloadProgressListener.js
Line: 78
When hitting the [x] on the download:
Error: uncaught exception: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIDownloadManager.cancelDownload]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://mozapps/content/downloads/downloads.js :: cancelDownload :: line 181"  data: no]

I was using private browsing in "keep my current session" mode, if that helps at all.

Comment 270

[Nochum Sossonko [:Natch]]

(In reply to comment #269)
Correction: Either I'll get that or the DM works fine, but on exit the DM retains the download data, from the wiki it seems the downloads should be reloaded from pre-PB mode. Also not the downloads aren't recorded in history (correctly).

Comment 271

[Aaron Train [:aaronmt]]

Hi,

I am a student looking into writing and or collaborating on unit tests for the patches you guys are submitting for private browsing mode.

Can someone direct me to or simply list the aspect(s) of Private Browsing mode that need testing (unit tests).

For example, Ehsan, I see you have listed some cases & testing areas - here 
https://wiki.mozilla.org/User:Ehsan/PrivateBrowsingTests#Unit_Tests

Are they in order of priority?

I'd appreciate any feedback, 
Thanks.

- AaronMT

Comment 272

[(no longer active)]

(In reply to comment #270)
> (In reply to comment #269)
> Correction: Either I'll get that or the DM works fine, but on exit the DM
> retains the download data, from the wiki it seems the downloads should be
> reloaded from pre-PB mode. Also not the downloads aren't recorded in history
> (correctly).

Can you reproduce this in a clean profile?  I can't reproduce it here.  What happens is that while the file is being downloaded, its progress should appear in the download manager, and when it's finished, it should be removed from the download manager.  I don't get errors on the console either...  Anyway, the functional spec has laid out new requirements for the download manager, so its implementation will change in the future revisions of my patch.

Comment 273

[(no longer active)]

(In reply to comment #271)
Aaron: thanks for your interest in helping out! :-)

The current test plan is being written by mconnor <https://wiki.mozilla.org/Firefox3.1/PrivateBrowsing/TestPlan>.  From the tests listed there, there are two which are not completed yet (I've marked the completed ones with a star next to their title).  I've a half-baked test for the unit tests section of the password manager, so that leaves out the history unit tests, which you can pick up to work on.

Another thing which needs testing is the private browsing service itself.  There is also an API for extensions which needs unit tests <https://wiki.mozilla.org/User:Ehsan/PrivateBrowsingForExtensions>.  It would be great if you can add a test plan for testing the APIs to <https://wiki.mozilla.org/Firefox3.1/PrivateBrowsing/TestPlan>, and start working on them as well.

Please let me know if you need any help (preferably via email, since this bug is already too huge).

Thanks!

Comment 274

[(no longer active)]

(In reply to comment #268)
> Annotations are notes that can be added by us or an extension developer to urls
> or bookmarks, so it can contain any data, personal or uri related data too (it
> depends on how the implementer uses it).

Thanks for the explanation.

> The main problem here is with page annotations, mainly because if an extension
> adds a page annotation during private browsing and the place_id does not exist
> we would probably try to add anno for place_id = -1 since you don't have a
> place.
> You should probably early return in nsAnnotationService::SetPageAnnotation
> while in private browsing mode for now, or check for place id validity later
> (better handling could be done though).

What about the typed versions of SetPageAnnotation?  They can be called through the nsIAnnotationService interface.  I guess a good solution here would be to change nsAnnotationService::GetPlaceIdForURI to ignore the auto create parameter if we're in the private browsing mode, and handle that case gracefully in all of its callers with auto create mode set.  Do you agree?

> about moz_inputhistory you should early return in
> nsNavHistory::AutoCompleteFeedback se we will not save what urls user has
> choosen in the awesomebar after searching for a certain text.

Thanks for mentioning this.  The next revision of the patch will include this change.

Comment 275

[(no longer active)]

(In reply to comment #208)
> * Instead of saving/restoring the list of closed tabs for all windows, I'd
> rather tag all tabs closed during private browsing mode and then just purge
> these when private browsing is over.

Simon: I tried to give this approach a shot, but it won't work, since we trim the _closedTabs array by max_tabs_undo all the time, so it's possible for some items in that array to get lost while in private mode, so after leaving this mode, the entire list as it was prior to entering this mode cannot be restored.

Comment 276

[Simon Bünzli]

(In reply to comment #275)
> it's possible for some items in that array to get lost while in private mode,

Actually, I wouldn't expect all the closed tabs to be still around after having used PB mode in a pre-existing window, in the same way as you don't expect the very same tabs to be around when you exit PB mode (you don't restore those to their original content, if the user didn't choose to close the original windows, do you?). So reducing or even completely clearing the list of recently closed tabs seems alright to me - otherwise you could reopen a tab in PB mode and reopen that same tab again after exiting PB mode, which strikes me as kind of odd.

Comment 277

[(no longer active)]

(In reply to comment #276)
> Actually, I wouldn't expect all the closed tabs to be still around after having
> used PB mode in a pre-existing window, in the same way as you don't expect the
> very same tabs to be around when you exit PB mode (you don't restore those to
> their original content, if the user didn't choose to close the original
> windows, do you?).

No, we don't, but come to think of it, maybe we should?  Mconnor: what do you think?

> So reducing or even completely clearing the list of recently
> closed tabs seems alright to me - otherwise you could reopen a tab in PB mode
> and reopen that same tab again after exiting PB mode, which strikes me as kind
> of odd.

What about the case where user chooses to open a new window to for the private browsing mode, and save and close her current session?

Comment 278

[Marco Bonardo [:mak]]

(In reply to comment #274)
> What about the typed versions of SetPageAnnotation?  They can be called through
> the nsIAnnotationService interface.  I guess a good solution here would be to
> change nsAnnotationService::GetPlaceIdForURI to ignore the auto create
> parameter if we're in the private browsing mode, and handle that case
> gracefully in all of its callers with auto create mode set.  Do you agree?

Well i could agree, notice that Shawn is trying to land the fsync stuff in these days (he will probably retry today), means that if things appear good (no build boxes failures and timings) you could move posting your visits/places into the temp tables, in that case the auto create change would not be useful because you could have a place already (hwv if i read code correctly we are not using the autoCreate actually).
But i suppose that GetPlaceIdForURI is still a good place to start hacking, in future you could change it to return -1 if the page is "private" and handle that case in type specific methods, and for now you can do the same, return -1 if in private mode and handle that.

Comment 279

[Simon Bünzli]

(In reply to comment #277)
> What about the case where user chooses to open a new window to for the private
> browsing mode, and save and close her current session?

Not closing any tabs in a pre-existing window won't modify the list of recently closed tabs, so you'll get the expected behavior. And closing/restoring all windows before entering/after exiting PB mode will restore the list of recently closed tabs with the windows, again as expected.

Comment 280

[(no longer active)]

(In reply to comment #278)
> Well i could agree, notice that Shawn is trying to land the fsync stuff in
> these days (he will probably retry today), means that if things appear good (no
> build boxes failures and timings) you could move posting your visits/places
> into the temp tables, in that case the auto create change would not be useful

I guess in that case, we could in fact not turns off anything in nsNavHistory, and just have everything use the temp tables, instead of the on-disk ones, right?

By the way, I'm not sure what APIs can be used to work with temp in-memory tables.  Is it something along the lines of doing a "PRAGMA temp_store=MEMORY;", creating temporary counterpart tables for each of the places tables, duplicating the data in on-disk tables on these new tables and having all the code use the new tables?

> because you could have a place already (hwv if i read code correctly we are not
> using the autoCreate actually).

autoCreate is set to true by default, so the function calls which don't specify it explicitly get the true default value.

> But i suppose that GetPlaceIdForURI is still a good place to start hacking, in
> future you could change it to return -1 if the page is "private" and handle
> that case in type specific methods, and for now you can do the same, return -1
> if in private mode and handle that.

I didn't get the difference between the "now" and "future" cases as you explained above, but that's what I'm going to do.  :-)

Comment 281

[Marco Bonardo [:mak]]

(In reply to comment #280)
> (In reply to comment #278)
> I guess in that case, we could in fact not turns off anything in nsNavHistory,
> and just have everything use the temp tables, instead of the on-disk ones,
> right?

by default we will add to the temp tables, and then sync on a timer or on bookmark-insert or on browser-exit to the disk, so 2 possibilities:
- don't sync to disk when we are in private browsing mode, clear temp tables on exit. Cons: this is complex to handle if we are allowing the user to create bookmarks while in private browsing mode since we would need to sync them down to disk.
- in private browsing mode mark places and visits with a "private" column in the temp table, on sync skip marked places/visits (this can be done easily modifying our triggers). Cons: needs temp table change and manage tables with different columns (disk table does not need a "private" column).

> By the way, I'm not sure what APIs can be used to work with temp in-memory
> tables.  Is it something along the lines of doing a "PRAGMA
> temp_store=MEMORY;", creating temporary counterpart tables for each of the
> places tables, duplicating the data in on-disk tables on these new tables and
> having all the code use the new tables?

using a temp table is simply done creating it like CREATE TEMP TABLE table_name, temp_store is not really useful (hwv we are already using it).
fsync stuff hwv inserts and updates in temp tables by default (yes we are partitioning places and visits table between disk and memory).

> I didn't get the difference between the "now" and "future" cases as you
> explained above, but that's what I'm going to do.  :-)

"now" is before fsync stuff, when you can't have a place. "future" is when you could have a place saved in a temp table. However imho you should start having "placeholders changes" in every place where you'll need to manager private browsing mode (that's what you're doing), impl can change later based on where we are moving to.

Comment 282

[(no longer active)]

Attachment: Patch (v2.9)

What's new in this revision:

 * Handled nsAnnotationService in private browsing mode, to make sure it doesn't store any annotations.
 * Handled nsNavHistory::AutoCompleteFeedback in private browsing mode, to make sure that user's choice in awesomebar does not get stored.
 * Corrected the behavior of the session store component on exit/restart.  Previously, if the user chose to restart the browser (for example, by installing an add-on) in private browsing mode, or closed the browser (provided that browser.startup.page is set to 3) without leaving the private mode, the private session was stored on disk, and would later be restored.  With this revision, this shouldn't happen any more, and the user's non-private session should be restored, as expected.

Comment 283

[(no longer active)]

(In reply to comment #281)
> - in private browsing mode mark places and visits with a "private" column in
> the temp table, on sync skip marked places/visits (this can be done easily
> modifying our triggers). Cons: needs temp table change and manage tables with
> different columns (disk table does not need a "private" column).

I think the second option can be used even without a new column, by using a new temp table and joining it against moz_places.

> using a temp table is simply done creating it like CREATE TEMP TABLE
> table_name, temp_store is not really useful (hwv we are already using it).
> fsync stuff hwv inserts and updates in temp tables by default (yes we are
> partitioning places and visits table between disk and memory).

Hmm, this page seems to suggest that temp tables are written to the disk while the DB connection is open <http://www.sqlite.org/tempfiles.html>.  Ideally we wouldn't want to do that.  According to the docs, temp_store might be able to save us there...

Comment 284

[Marco Bonardo [:mak]]

(In reply to comment #283)
> (In reply to comment #281)
> I think the second option can be used even without a new column, by using a new
> temp table and joining it against moz_places.

we can't, it's already quite difficult having one temp table, having two would be a pain. that's because sqlite does not support indices on unions or views, so we have to use very specialized queries to read from both (disk and memory).

> Hmm, this page seems to suggest that temp tables are written to the disk while
> the DB connection is open <http://www.sqlite.org/tempfiles.html>.  Ideally we
> wouldn't want to do that.  According to the docs, temp_store might be able to
> save us there...

temp_store is memory in Places

Comment 285

[Jim Jeffery not reading bug-mail 1/2/11]

After playing with a couple of builds I went back to official nightly builds.  Today I discovered that new URL's were not being shown when entered into the Location bar.  

Examination with History->Show all history, did show the URL but the visit count was zero.  I got to looking at other history of my normal visited sites and found that none were being 'counted as visited' anymore. 

I renamed Places.sqlite to places.sqlite.old and restarted the browser to find that my bookmarks were still there ?  My history was indeed gone, and is now being 'counted as visisted and incrementing' as it should.  

Bug or ?  Almost seems that I was stuck in PB mode.  Honestly I cannot recall if I properly exited PB mode before going back to official nightly's.

Comment 286

[(no longer active)]

Jim: have you tried to reproduce this?  Your comment was a bit vague, and I need some STR in order to try to debug this.

Comment 287

[Jim Jeffery not reading bug-mail 1/2/11]

No, I just discovered this at the time of my comment #286 above.  I had to leave for work, so will be tomorrow, or wednesday before I have time to install the PB tryserver build and see if things break again.  

Can you explain what seems to vague?  

The issue seems to be that none of the sites I visit are being shown in the Library with a 'visit count'.  The URL is there but the visit count is zero.  As long as the count stays at zero of course it won't show in the Location bar when doing a search.

Comment 288

[Marcia Knous [:marcia]]

I tried to reproduce what Jim is seeing in Comment 285 and was not able to do so, but I was testing with a fresh profile. Here is what I did:

1. Downloaded the latest private browsing tryserver build
2. Created a fresh profile.
3. Surfed 4-5 sites than invoked PB mode. I left the other browsing session open by selecting that preference.
4. Browsed 4-5 sites in PB mode.
5. Exited the browser and started up the latest trunk nightly using the same profile.
6. Browsed to the sites that in regular mode and revisited. The visit count incremented correctly and the sites showed up in the location bar.

What I did not do is use an existing profile that had already been run on the trunk to see what happens, I can do that next, since that is probably closer to what Jim was doing.

Comment 289

[Jim Jeffery not reading bug-mail 1/2/11]

Yes I was using a months old profile that I normally keep history for 90 days. 

Thanks for testing Marcia.  While your at it, could you also try:

Open the browser, enter PB mode, then close and re-install a current official nightly build?  I'm really suspecting that something was left 'locked' in my places.sqlite file, thus leaving the browser 'thinking' it was still in PB Mode.

I have SQLite browser, but I'm not sure what to look for, and will be tomorrow before I can even do that.

Comment 290

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Initiated a new try server build with patch v2.9. Should be available in an hour or so.

Comment 291

[Marcia Knous [:marcia]]

I tested the scenario that Jim outlined in Comment 289 using a Windows Vista VM. Again, here are the steps I followed:

1. Downloaded and installed the latest tryserver build.
2. Entered PB mode and surfed a few sites. Exited FF still in PB mode.
3. Ran the uninstaller and uninstalled the tryserver build.
4. Installed the latest trunk nightly (Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b1pre) Gecko/20080922032349 Minefield/3.1b1pre) and ran with the same profile.

Result: Everything seemed okay as far as being able to access sites and the visit counts registered appropriately. The sites that I visited in PB mode **did** show up in history when I relaunched using the trunk, which I am not certain is expected or not - will this present an issue for individuals going back and forth between builds once the feature is launched?

Comment 292

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Latest try server builds come in here: