Opened 4 years ago

Closed 4 years ago

Last modified 4 months ago

#12620 closed task (fixed)

Rebase TBB patches to Firefox 31 and add unit tests

Reported by: gk Owned by: tbb-team
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Blocker Keywords: ff31-esr, tbb-rebase, tbb-firefox-patch, TorBrowserTeam201409D, tbb-no-uplift
Cc: intrigeri, arthuredelstein@…, boklm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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.

Child Tickets

TicketStatusOwnerSummaryComponent
#12873closeddcfReenable TLSv1.1 and TLSv1.2 in meek-http-helper when rebased on Firefox 31 ESRObfuscation/meek

Attachments (1)

0001-fixup-Bug-6253-return-white-placeholder-image-data.patch (9.6 KB) - added by mcs 4 years ago.
canvas patch fixup

Download all attachments as: .zip

Change History (47)

comment:1 Changed 4 years ago by intrigeri

Cc: intrigeri added

comment:2 Changed 4 years ago by arthuredelstein

Cc: arthuredelstein@… added

comment:3 Changed 4 years ago by arthuredelstein

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.

Last edited 4 years ago by arthuredelstein (previous) (diff)

comment:4 Changed 4 years ago by arthuredelstein

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!

comment:5 Changed 4 years ago by erinn

Component: Firefox Patch IssuesTor Browser
Keywords: tbb-firefox-patch added
Owner: changed from mikeperry to tbb-team

comment:6 Changed 4 years ago by mikeperry

Keywords: TorBrowserTeam201408 added

comment:7 Changed 4 years ago by mikeperry

Keywords: TorBrowserTeam201408D added; TorBrowserTeam201408 removed

comment:8 Changed 4 years ago by mikeperry

Priority: normalmajor

comment:9 Changed 4 years ago by arthuredelstein

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

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.

comment:10 Changed 4 years ago by boklm

Cc: boklm added

comment:11 Changed 4 years ago by arthuredelstein

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

Last edited 4 years ago by arthuredelstein (previous) (diff)

comment:12 in reply to:  11 Changed 4 years ago by arthuredelstein

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

comment:13 Changed 4 years ago by arthuredelstein

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:

Here are my notes and questions when porting to ESR31:

  • Bug #2874: New version of this patch due to a previous regression
  • Bug #2875: Are there new Media Queries we should block?
  • Bug #2872: Comments say "XXX: Fallback is bad" and there are several other XXX items.
  • Bug #5715: Comment says " XXX: Bloody hack until we get this notifier in FF14.0". It looks like this patch needs revision.
  • Bug #5741: I'm not sure whether or not to add the extra lines in netwerk/protocol/websocket/WebSocketChannel.cpp
  • Bug #4755: I rewrote this patch -- needs to be tested carefully.
  • Bug #6253: 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: A lot of porting work. Needs careful inspection.
  • Bug #5282: FIXME comment in nsHttpConnectionMgr.cpp. A couple of minor anomalies whimage/src/imgLoader.hen rebasing -- needs to be tested.* Bug #10159: 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: 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: I'm a little unsure if I did this correctly -- needs to be tested on all platforms.
  • Bug #9701: Should we attach this to a pref?
  • Bug #10819: I took parts of this patch and used them to fixup #6539 and #6564. What remains is the definition of the pref itself and some changes to ThirdPartyUtils. I also moved #10819 to upstream of #6539 and #6564.
  • Bug #10895: 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.

Last edited 4 years ago by arthuredelstein (previous) (diff)

comment:14 Changed 4 years ago by gk

Okay, let's start:

1) https://github.com/arthuredelstein/tor-browser/commit/768f2c0dbad0dbf1947211804a6446fee2f20a55 is not needed anymore. We got that into Firefox See: #10716 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 for the background and the remaining #11189 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)

And you meant #9128 above, right ;)

comment:15 in reply to:  14 Changed 4 years ago by arthuredelstein

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 above, right ;)

Oops! Edited for clarity.

comment:16 Changed 4 years ago by arthuredelstein

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

comment:17 Changed 4 years ago by gk

Two other thoughts:

1) What is the rationale for including the patch for #2874 into ESR 31? Reading #2874 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. 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).

comment:18 in reply to:  17 ; Changed 4 years ago by arthuredelstein

Replying to gk:

1) What is the rationale for including the patch for #2874 into ESR 31? Reading #2874 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. 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! :)

comment:19 in reply to:  17 Changed 4 years ago by mikeperry

Replying to gk:

Two other thoughts:

1) What is the rationale for including the patch for #2874 into ESR 31? Reading #2874 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. 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 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.

comment:20 Changed 4 years ago by arthuredelstein

OK, I'm leaving the two patches in for now.

comment:21 Changed 4 years ago by arthuredelstein

I have a branch of torbutton.git with some fixes to get it working (at least superficially) on the esr31-port-untested build: https://github.com/arthuredelstein/torbutton/commits/12620.

comment:22 Changed 4 years ago by arthuredelstein

comment:23 Changed 4 years ago by arthuredelstein

In trying to apply "Backport two integer overflow patches, I found that the part in Interpreter-inl.h doesn't apply to ESR31. The code in the Interpreter has changed substantially. I think that the ESR31 version of AddOperation does not protect overflow (which looks like a possible security regression for Mozilla), but it will require careful study to understand.

The rest of the patch is already in ESR31.

Last edited 4 years ago by arthuredelstein (previous) (diff)

comment:24 Changed 4 years ago by arthuredelstein

comment:26 in reply to:  23 ; Changed 4 years ago by gk

Replying to arthuredelstein:

In trying to apply "Backport two integer overflow patches, I found that the part in Interpreter-inl.h doesn't apply to ESR31. The code in the Interpreter has changed substantially. I think that the ESR31 version of AddOperation does not protect overflow (which looks like a possible security regression for Mozilla), but it will require careful study to understand.

Those overflows are fixed in ESR 31. So, nothing to backport here.

comment:27 Changed 4 years ago by mikeperry

Keywords: TorBrowserTeam201409D added; TorBrowserTeam201408D removed

comment:28 in reply to:  26 Changed 4 years ago by arthuredelstein

Replying to gk:

Replying to arthuredelstein:

In trying to apply "Backport two integer overflow patches, I found that the part in Interpreter-inl.h doesn't apply to ESR31. The code in the Interpreter has changed substantially. I think that the ESR31 version of AddOperation does not protect overflow (which looks like a possible security regression for Mozilla), but it will require careful study to understand.

Those overflows are fixed in ESR 31. So, nothing to backport here.

OK, this patch has been dropped.

Changed 4 years ago by mcs

canvas patch fixup

comment:29 Changed 4 years ago by mcs

brade and I reviewed the canvas patch (bug #6253). Everything looks good, except we think it would be better to return all-white image placeholder data (to be compatible with the behavior of TB 3.6.x)... hence the fixup patch I just attached to this bug.

comment:30 in reply to:  29 Changed 4 years ago by arthuredelstein

Replying to mcs:

brade and I reviewed the canvas patch (bug #6253). Everything looks good, except we think it would be better to return all-white image placeholder data (to be compatible with the behavior of TB 3.6.x)... hence the fixup patch I just attached to this bug.

Thanks! Added to https://github.com/arthuredelstein/tor-browser/commits/12620.

(Edit: corrected URL)

Last edited 4 years ago by arthuredelstein (previous) (diff)

comment:31 Changed 4 years ago by mcs

brade and I reviewed and tested the DOM storage patch (bug #6564). Everything looks good.

comment:32 Changed 4 years ago by gk

comment:33 in reply to:  32 Changed 4 years ago by arthuredelstein

Replying to gk:

We don't need https://github.com/arthuredelstein/tor-browser/commit/a1340ec288021736e9905bd345499e192ec33c45 anymore. Arthur, could you revert it?

Done.

comment:34 Changed 4 years ago by gk

comment:35 in reply to:  34 Changed 4 years ago by arthuredelstein

Replying to gk:

We don't need https://github.com/arthuredelstein/tor-browser/commit/c1b1884060f603158167eee07ba5cb1c770b8429 anymore. Arthur, could you revert it?

Done.

comment:36 in reply to:  18 ; Changed 4 years ago by gk

Replying to arthuredelstein:

Replying to gk:

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?

Done: #13180. But in order to use this the blocks starting with PRNetAddr tempAddr; need to come after notifying the observer. This is okay, there won't be DNS proxy bypasses due to it. While I am at it: the second PRNetAddr tempAddr; got merged wrongly. It has to be included in Resolve() and not in AsyncResolve() (there, the first of those blocks is already doing its job). The added code in WebSocketChannel.cpp (in https://github.com/arthuredelstein/tor-browser/commit/1f1d39cd64da97a1d19857dc27c7b5ba059dd863) is not needed in ESR 31 anymore. It can get removed.

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?

Done: #13181.

comment:37 in reply to:  36 ; Changed 4 years ago by arthuredelstein

Replying to gk:

Replying to arthuredelstein:

Replying to gk:

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?

Done: #13180. But in order to use this the blocks starting with PRNetAddr tempAddr; need to come after notifying the observer. This is okay, there won't be DNS proxy bypasses due to it. While I am at it: the second PRNetAddr tempAddr; got merged wrongly. It has to be included in Resolve() and not in AsyncResolve() (there, the first of those blocks is already doing its job). The added code in WebSocketChannel.cpp (in https://github.com/arthuredelstein/tor-browser/commit/1f1d39cd64da97a1d19857dc27c7b5ba059dd863) is not needed in ESR 31 anymore. It can get removed.

Oops -- thanks for catching that mistake. I've added a fixup here:
https://github.com/arthuredelstein/tor-browser/commit/8739f05ae42509a661c01426314c8af165e43594

Please let me know if this looks right to you.

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?

Done: #13181.

Thanks!

comment:38 in reply to:  37 ; Changed 4 years ago by gk

Replying to arthuredelstein:

Replying to gk:

Replying to arthuredelstein:

Replying to gk:

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?

Done: #13180. But in order to use this the blocks starting with PRNetAddr tempAddr; need to come after notifying the observer. This is okay, there won't be DNS proxy bypasses due to it. While I am at it: the second PRNetAddr tempAddr; got merged wrongly. It has to be included in Resolve() and not in AsyncResolve() (there, the first of those blocks is already doing its job). The added code in WebSocketChannel.cpp (in https://github.com/arthuredelstein/tor-browser/commit/1f1d39cd64da97a1d19857dc27c7b5ba059dd863) is not needed in ESR 31 anymore. It can get removed.

Oops -- thanks for catching that mistake. I've added a fixup here:
https://github.com/arthuredelstein/tor-browser/commit/8739f05ae42509a661c01426314c8af165e43594

Please let me know if this looks right to you.

Better. Please move the first block beginning with PRNetAddr tempAddr; after

if (mNotifyResolution) {
        NS_DispatchToMainThread(new NotifyDNSResolution(mObserverService,
                                                        hostname));
}

in AsyncResolve(), too. Otherwise we can't do #13180.

comment:39 in reply to:  38 Changed 4 years ago by arthuredelstein

Replying to gk:

Replying to arthuredelstein:

Replying to gk:

Please let me know if this looks right to you.

Better. Please move the first block beginning with PRNetAddr tempAddr; after

if (mNotifyResolution) {
        NS_DispatchToMainThread(new NotifyDNSResolution(mObserverService,
                                                        hostname));
}

in AsyncResolve(), too. Otherwise we can't do #13180.

Done: https://github.com/arthuredelstein/tor-browser/commit/e439d265421d28be79ed5ebc1bb900b7f94ee4bd

comment:40 Changed 4 years ago by arthuredelstein

For the record, here's our commits on ESR 31, with fixups squashed:
https://github.com/arthuredelstein/tor-browser/commits/12620D
Next I'll rebase to Firefox 31.1.0 ESR.

comment:41 Changed 4 years ago by arthuredelstein

comment:42 Changed 4 years ago by arthuredelstein

comment:43 in reply to:  42 Changed 4 years ago by arthuredelstein

Replying to arthuredelstein:

Rebased again to Firefox 31.1.1 ESR:
https://github.com/arthuredelstein/tor-browser/commits/tbb-esr31.1.1

I've now added patches 13047 and 13091. Looks like 13049 is already applied in another patch by Kathy.

comment:44 Changed 4 years ago by mikeperry

Resolution: fixed
Status: newclosed

I've merged this into tor-browser.git/tor-browser-31.1.1esr-4.x-1. Calling this fixed. We will file new tickets for any future changes, rather than using this bug.

comment:45 Changed 4 years ago by arthuredelstein

To double-check that no patches were lost, I have done a manual patch-by-patch comparison of
https://gitweb.torproject.org/tor-browser.git/shortlog/refs/heads/tor-browser-31.1.1esr-4.x-1
and
https://gitweb.torproject.org/tor-browser.git/shortlog/refs/heads/tor-browser-24.8.1esr-4.x-1

I confirmed that all patches present in 24.8.1 are accounted for (either present in 31.1.1 or obsolete).

comment:46 Changed 4 months ago by arthuredelstein

Keywords: tbb-no-uplift added
Severity: Blocker
Note: See TracTickets for help on using tickets.