Opened 4 years ago

Last modified 9 months ago

#9570 needs_revision defect

Many changes to private browsing code of Firefox happened since 17esr out

Reported by: cypherpunks Owned by: mikeperry
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: MikePerry201312R
Cc: g.koppen@…, mcs, brade Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

https://developer.mozilla.org/en-US/docs/Updating_addons_broken_by_private_browsing_changes

Torbutton will be broken by Firefox24ESR private browsing changes.

Child Tickets

Change History (12)

comment:1 Changed 4 years ago by gk

Cc: g.koppen@… added

comment:2 Changed 4 years ago by mcs

Cc: mcs brade added

comment:3 Changed 4 years ago by mcs

Status: newneeds_review

We reviewed the Torbutton code and the Mozilla changes and made a collection of small fixes:

https://gitweb.torproject.org/user/brade/torbutton.git/commit/bc00d2adc4b27469f000c121afbed7e30f53b553

Mike, please review.

Also, some of the old Torbutton JS components generate "noise" when reviewing this kind of large of change by Mozilla, so we removed several files on one of our branches. If you want to take it, that commit is here:

https://gitweb.torproject.org/user/brade/torbutton.git/commit/6a5998a531d29374d7d0e2b1dfb4f31159648a6a

comment:4 in reply to:  3 ; Changed 4 years ago by cypherpunks

We reviewed the Torbutton code and the Mozilla changes and made a collection of small fixes:

What about torbutton.js: getService(Components.interfaces.imgICache);
https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/imgICache

comment:5 in reply to:  4 Changed 4 years ago by mcs

Status: needs_reviewneeds_revision

Replying to cypherpunks:

What about torbutton.js: getService(Components.interfaces.imgICache);
https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/imgICache

Somehow we missed that... sorry. We are working on a new patch. Firefox now has two image caches (regular and private browsing). Unfortunately, there does not seem to be a way to clear the contents of the private browsing image cache unless you have access to a document that is part of a private browsing session.

comment:6 Changed 4 years ago by mcs

Status: needs_revisionneeds_review

comment:7 Changed 4 years ago by mikeperry

Keywords: MikePerry201311R added

comment:8 Changed 4 years ago by mikeperry

Status: needs_reviewneeds_revision

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?
  2. Related: We're sure there can be only two image caches, right?
  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?
  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?

comment:9 Changed 4 years ago by mikeperry

Keywords: MikePerry201312R added; MikePerry201311R removed

comment:10 in reply to:  8 Changed 4 years ago by mcs

Replying to 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.

  1. 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

  1. 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

  1. 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.

comment:11 Changed 9 months ago by cypherpunks

Component: TorBrowserButtonApplications/Tor Browser
Keywords: ff52-esr added; ff24-esr removed
Severity: Normal

comment:12 Changed 9 months ago by gk

Keywords: ff52-esr removed
Note: See TracTickets for help on using tickets.