Rebase TBB patches to Firefox 31 and add unit tests

As early as possible, we should begin rebasing our patches to Firefox 31 so we can understand the scope of the work that will be involved in updating them.

added TorBrowserTeam201409D component::applications/tor browser ff31-esr owner::tbb-team priority::high resolution::fixed severity::blocker status::closed tbb-firefox-patch tbb-no-uplift tbb-rebase type::task labels

Trac:
Cc: N/A to intrigeri

Trac:
Cc: intrigeri to intrigeri, arthuredelstein@gmail.com

Ahead of this rebase, I was thinking it would be nice to have a torproject trac bug label prepended to every tor-browser.git commit message, the same way Mozilla prepends Bugzilla bug numbers in their commits. That way we can more easily keep track of patches as they get ported one by one to ESR 31. I'm happy to try to create this on a new branch if Mike and the TB team think it's a good idea.

I was able to find Bug IDs for all but 5 patches.

https://github.com/arthuredelstein/tor-browser/commits/bugIDs-tor-browser-24.7.0esr-4.x-1

To get these commit messages with bugIDs, you can add https://github.com/arthuredelstein/tor-browser.git as a remote and then make a local copy of the bugIDs-tor-browser-24.7.0esr-4.x-1 branch.

To confirm that nothing has changed (except commit messages), I compared branches before and after changing commit messages:

/p/t/tor-browser> git diff tor-browser-24.7.0esr-4.x-1..bugIDs-tor-browser-24.7.0esr-4.x-1 
/p/t/tor-browser> 

For the five missing Bug IDs, I added an arbitrary label (TB1...TB5). Feel free to replace these with Bug IDs if they exist!

Trac:
Keywords: N/A deleted, tbb-firefox-patch added
Component: Firefox Patch Issues to Tor Browser
Owner: mikeperry to tbb-team

Trac:
Keywords: N/A deleted, TorBrowserTeam201408 added

Trac:
Keywords: TorBrowserTeam201408 deleted, TorBrowserTeam201408D added

Trac:
Priority: normal to major

I'm changing the title here to include unit tests. I think, as we're reviewing these patches and porting them to ESR31, it will be a good opportunity to add unit tests wherever possible, to reduce the likelihood of regressions now or in the future. I would suggest we add these unit tests to the general Mozilla testing suite, so that when offer upstream patches to Mozilla, it will include our tests. Of course, we'll also need to make sure existing tests for ESR31 don't break or are patched.

Trac:
Summary: Rebase TBB patches to Firefox 31 to Rebase TBB patches to Firefox 31 and add unit tests

Trac:
Cc: intrigeri, arthuredelstein@gmail.com to intrigeri, arthuredelstein@gmail.com, boklm

I propose we drop Bug #2874 (closed) patch altogether (see https://trac.torproject.org/projects/tor/ticket/2874#comment:15) for the port to ESR31.

I've changed my mind -- I think we should remove Components.interfaces (for #2874 (closed)) in ESR31 after all.

I've now rebased all patches in the tor-browser-24.7.0esr-4.x-2 branch of tor-browser.git, up to 3ef809fc, on top of ESR31 branch. The resulting branch is here: esr31-ported-untested.

Edit: the latest TBB/ESR31 branch is now https://github.com/arthuredelstein/tor-browser/commits/12620

Except where noted (below), patches are in the same order in the TB-ESR24 and TB-ESR31 branches.

Patches in TB-ESR24 branch omitted in TB-ESR31 branch:

• Bug #8987 (closed): Already in Mozilla ESR31 as Bug 967970.
• Bug #9128 (moved): Already in Mozilla ESR31 as Bug 962314.
• Bug #9829 (closed).2: Already in Mozilla ESR31 as Bug 931053.
• Bug #9829 (closed).3: Already in Mozilla ESR31 as Bug 932127.
• Bug #9829 (closed).5: Already in Mozilla ESR31 as Bug 931043.
• Bug #9830 (closed): Already in Mozilla ESR31 as Bug 763070, Bug 880836, Bug 887828, Bug 887821.
• Bug #9308 (closed): Obsolete in ESR31; not included.

Here are my notes and questions when porting to ESR31:

• Bug #2874 (closed): New version of this patch due to a previous regression
• Bug #2875 (closed): Are there new Media Queries we should block?
• Bug #2872 (closed): Comments say "XXX: Fallback is bad" and there are several other XXX items.
• Bug #5715 (closed): Comment says "// XXX: Bloody hack until we get this notifier in FF14.0". It looks like this patch needs revision.
• Bug #5741 (closed): I'm not sure whether or not to add the extra lines in netwerk/protocol/websocket/WebSocketChannel.cpp
• Bug #4755 (closed): I rewrote this patch -- needs to be tested carefully.
• Bug #6253 (closed): When porting this bug, I changed returned placeholder canvas data from all-white to all-black, because I found retaining the all-white very difficult given the new Mozilla code structure, while all-black is very simple. Also applied recent fixups from Isis.
• Bug #6539 (closed): A lot of porting work. Needs careful inspection.
• Bug #5282 (closed): FIXME comment in nsHttpConnectionMgr.cpp. A couple of minor anomalies whimage/src/imgLoader.hen rebasing -- needs to be tested.* Bug #10159 (closed): Some of this patch already landed upstream. JarMaker.py and js/src/config/expandlibs_exec.py seem to have disappeared. Applied the remaining parts.
• Bug #10139 (closed): Fixed upstream, though js/src/config/rules.mk has vanished. I fixed analogous ENABLE_STRIP references in ./nsprpub/config/rules.mk. Is this the right thing to do?
• Bug #10252 (closed): I'm a little unsure if I did this correctly -- needs to be tested on all platforms.
• Bug #9701 (moved): Should we attach this to a pref?
• Bug #10819 (closed): I took parts of this patch and used them to fixup #6539 (closed) and #6564 (closed). What remains is the definition of the pref itself and some changes to ThirdPartyUtils. I also moved #10819 (closed) to upstream of #6539 (closed) and #6564 (closed).
• Bug #10895 (closed): I made the analogous change in browser/config/version.txt from "31.0esrpre" to "31.0". Is this needed? Not sure how to test.

So I think the next step should be to manually test each patch on the new ESR31-based branch, and add unit tests wherever possible. Any comments or fixups to patches on the esr31-ported-untested branch will be very welcome at this stage.

Okay, let's start:

1. https://github.com/arthuredelstein/tor-browser/commit/768f2c0dbad0dbf1947211804a6446fee2f20a55 is not needed anymore. We got that into Firefox See: #10716 (moved) for the background and the remaining things to do.
2. https://github.com/arthuredelstein/tor-browser/commit/066d0c2aadb48eaafed7191975a07ea60258734c is not needed anymore. We got that fixed upstream: https://bugzilla.mozilla.org/show_bug.cgi?id=981558
3. https://github.com/arthuredelstein/tor-browser/commit/63d2ba95d629baee7e6846842e89f9810f4b8335 is not needed anymore. We got all the deterministic build fixes into Firefox. See: #10111 (closed) for the background and the remaining #11189 (moved) before we switch to ESR 31.
4. https://github.com/arthuredelstein/tor-browser/commit/ef637032fbc5b5c37b5a5e62610d3fecfcb760ec is not needed I think. NSPR is a different beast, thus I'd rader avoid messing with it unless we really have to. (which answers your question re #10139 (closed))

And you meant #9128 (moved) above, right ;)

Replying to gk:

Okay, let's start: [...]

OK, I've committed reverts to those four patches (when we rebase they can be removed).

And you meant #9128 (moved) above, right ;)

Oops! Edited for clarity.

For Bug #2875 (closed), I looked and couldn't find any new media queries to block. Unit tests have been added.

Two other thoughts:

1. What is the rationale for including the patch for #2874 (closed) into ESR 31? Reading #2874 (closed) it seems to me the issue is resolved in ESR 31 by Mozilla itself and comment:16:ticket:2874 does not convince me (yet) as there are numerous ways to distinguish ESR 24 and ESR 31 users and we don't aim at making them undistinguishable. I'd like to see the patches we need as a kind of roadmap of things still in need of getting upstreamed and I wonder how the patch in question would fit into that picture (but, admittedly, maybe the picture is wrong to begin with)...

2. I wonder whether we should still include the patch for #5741 (closed). For one, Mozilla fixed that leak (https://bugzilla.mozilla.org/show_bug.cgi?id=751465). Then, we added a unit test making sure that nothing gets backed out wrt the WebSocket protocol which leads to another round of DNS bypassing tor (https://bugzilla.mozilla.org/show_bug.cgi?id=971153). Now, we can even observe the respective notification in Torbutton to be extra sure that no leaks happening (might be a good QA thing...). The only argument for including the patch in ESR 31 I currently can come up with is that ws:// is the only protocol currently being tested in the unit test. If that is a show-stopper, fair enough (I planned to add tests for the remaining non-internal protocols + getting them merged into ESR 38).

Replying to gk:

1. What is the rationale for including the patch for #2874 (closed) into ESR 31? Reading #2874 (closed) it seems to me the issue is resolved in ESR 31 by Mozilla itself and comment:16:ticket:2874 does not convince me (yet) as there are numerous ways to distinguish ESR 24 and ESR 31 users and we don't aim at making them undistinguishable. I'd like to see the patches we need as a kind of roadmap of things still in need of getting upstreamed and I wonder how the patch in question would fit into that picture (but, admittedly, maybe the picture is wrong to begin with)...

A comment in the Mozilla code suggests that Components.interfaces will be removed at some point in the future, so in a sense this will be upstreamed. But I see your point that we can't hope to make ESR24 and ESR31 indistinguishable. So I think I'm persuaded that this patch can be removed. I'll add a revert commit.

2. I wonder whether we should still include the patch for #5741 (closed). For one, Mozilla fixed that leak (https://bugzilla.mozilla.org/show_bug.cgi?id=751465). Then, we added a unit test making sure that nothing gets backed out wrt the WebSocket protocol which leads to another round of DNS bypassing tor (https://bugzilla.mozilla.org/show_bug.cgi?id=971153).

Very nice!

Now, we can even observe the respective notification in Torbutton to be extra sure that no leaks happening (might be a good QA thing...).

Definitely. Is there a ticket for this?

The only argument for including the patch in ESR 31 I currently can come up with is that ws:// is the only protocol currently being tested in the unit test. If that is a show-stopper, fair enough (I planned to add tests for the remaining non-internal protocols + getting them merged into ESR 38).

Maybe open a ticket for those additional unit tests? Happy to revert this patch if you think it's not needed.

Thanks for your help! :)

Replying to gk:

Two other thoughts:

1. What is the rationale for including the patch for #2874 (closed) into ESR 31? Reading #2874 (closed) it seems to me the issue is resolved in ESR 31 by Mozilla itself and comment:16:ticket:2874 does not convince me (yet) as there are numerous ways to distinguish ESR 24 and ESR 31 users and we don't aim at making them undistinguishable. I'd like to see the patches we need as a kind of roadmap of things still in need of getting upstreamed and I wonder how the patch in question would fit into that picture (but, admittedly, maybe the picture is wrong to begin with)...

Mozilla also sometimes makes interface changes in point releases, and interfaces can vary between platforms and OS versions. I think we want this entire Components.* hierarchy gone, not just defanged.

2. I wonder whether we should still include the patch for #5741 (closed). For one, Mozilla fixed that leak (https://bugzilla.mozilla.org/show_bug.cgi?id=751465). Then, we added a unit test making sure that nothing gets backed out wrt the WebSocket protocol which leads to another round of DNS bypassing tor (https://bugzilla.mozilla.org/show_bug.cgi?id=971153). Now, we can even observe the respective notification in Torbutton to be extra sure that no leaks happening (might be a good QA thing...). The only argument for including the patch in ESR 31 I currently can come up with is that ws:// is the only protocol currently being tested in the unit test. If that is a show-stopper, fair enough (I planned to add tests for the remaining non-internal protocols + getting them merged into ESR 38).

They fixed the WebSockets leak, but the second part of the #5741 (closed) patch was to ensure against any additional forms of DNS leak. It also saved our users from being told by StartPage to enter www.startpage.com in "no proxies for" line of the proxy settings, since not being able to do that resolution prevented that from working.

I think for defense in depth, we should keep the DNS service piece of patch.