[tor-bugs] #9570 [TorBrowserButton]: Many changes to private browsing code of Firefox happened since 17esr out

[Tor Bug Tracker & Wiki]

#9570: Many changes to private browsing code of Firefox happened since 17esr out
----------------------------------+----------------------------------------
     Reporter:  cypherpunks       |      Owner:  mikeperry
         Type:  defect            |     Status:  needs_revision
     Priority:  normal            |  Milestone:
    Component:  TorBrowserButton  |    Version:
   Resolution:                    |   Keywords:  ff24-esr, MikePerry201312R
Actual Points:                    |  Parent ID:
       Points:                    |
----------------------------------+----------------------------------------

Comment (by mcs):

 Replying to [comment:8 mikeperry]:
 > I went ahead and merged this because it is an improvement and will help
 us test FF24 sooner, but I have the following questions and concerns:
 >
 > 1. For the image cache stuff: Do we need move that call up to before the
 point where we try to close any windows? On the one hand, we don't want to
 miss clearing it if there are no private windows currently open (because
 we just closed them). On the other hand, we don't want to let any windows
 load any images after we've cleared the image cache.. I'm also surprised
 that closing all windows doesn't clear the private browsing data + caches
 automatically. Shouldn't it?

 The code in torbutton_close_on_toggle() closes all tabs but opens an
 about:blank tab in each open window, and it closes all windows except the
 one that torbutton_do_new_identity() is running in.  If the remaining
 window is a private browsing one, the code in
 torbutton_clear_image_caches() will take care of clearing the private
 browsing cache.  If the last remaining window is a regular one, then
 Firefox will have already cleared the private browsing cache by the time
 torbutton_clear_image_caches() is called.

 > 2. Related: We're sure there can be only two image caches, right?

 Yes.  See the last return statement in
 nsContentUtils::GetImgLoaderForDocument().
 http://mxr.mozilla.org/mozilla-
 esr24/source/content/base/src/nsContentUtils.cpp#2604

 > 3. For the nsIContentPrefService2 clearing code, do we need to get
 contexts for both private and non-private windows here too? It sounds like
 we might?

 It looks like removeAllDomains() clears the regular store and then, if you
 pass in a private browsing context, it also clears the PB store.
 http://mxr.mozilla.org/mozilla-
 esr24/source/toolkit/components/contentprefs/ContentPrefService2.jsm#461

 > 4. For your comment about {private: true} above OpenBrowserWindow(),
 that sounds like it might be our fix for #8400, yes? If we set that arg
 based on the torbutton pref extensions.torbutton.block_disk, perhaps that
 will solve #8400, right?

 The window created here (during new identity) should have the correct
 private browsing characteristics (the new window will "follow" the value
 of browser.privatebrowsing.autostart).  Regarding 8400, our reading of the
 Firefox code indicates that a restart is NOT required -- except existing
 open windows will retain their old state with respect to private browsing.
 That means that if someone starts out with "Don't record browsing history"
 off and then turns it on, the windows that were already open will still be
 recording history.  I suspect that is why Mozilla forces a restart when
 users toggle browser.privatebrowsing.autostart via the preferences UI.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/9570#comment:10>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online