Opened 5 years ago

Closed 5 years ago

Last modified 6 months ago

#9173 closed defect (fixed)

Relocate RelativeLink functionality to Firefox patch

Reported by: mikeperry Owned by: mcs
Priority: Very High Milestone:
Component: Firefox Patch Issues Version:
Severity: Keywords: tbb-usability, tbb-3.0-backport, MikePerry201311R, tbb-no-uplift
Cc: ioerror, brade Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We need to hardcode our home and profile settings from RelativeLink into a Firefox patch so that users still get expected behavior on Mac and Windows if they dock Firefox instead of the RelativeLink exe/containing app.

This will also prevent situations where TBB ends up set as the default browser or link handler, and gets launched to handle a url without RelativeLink.

This is a pretty bad usability bug, because Firefox will end up using the wrong profile if you launch it without RelativeLink right now.

In most cases, this will cause it to fail closed (because the socks proxy won't be listening), but not if the user already has proxy settings overrides for their default profile.

Child Tickets

Attachments (2)

0001-Use-firefox-bin.patch (1.4 KB) - added by arlolra 5 years ago.
9173-profile-location-patch.txt (14.9 KB) - added by mcs 5 years ago.
proposed fix (change Firefox user data location)

Download all attachments as: .zip

Change History (25)

comment:1 Changed 5 years ago by mikeperry

Possibly as part of this, we may want to add code that checks to ensure that the profile we're pointed actually has Torbutton and/or Tor Launcher addons in it, however we probably want to do those checks separately and carefully, or they will break things like Tails.

Changed 5 years ago by arlolra

Attachment: 0001-Use-firefox-bin.patch added

comment:2 Changed 5 years ago by arlolra

Status: newneeds_review

Did you try using firefox-bin? Please see the attached.

comment:3 Changed 5 years ago by mikeperry

Keywords: MikePerry201307 added

Hrmm, no I didn't. Sounds plausible. Did you try this out?

Also, this probably won't help Windows users, but if it works, we'll just file a new ticket for them.

comment:4 Changed 5 years ago by arlolra

Yes, I tested and it works for me on OS X 10.8.4.
Correct, this doesn't help Windows.

comment:5 Changed 5 years ago by arlolra

Although, it doesn't address the issue of the default browser / link handler. Just the dock icon.

comment:6 Changed 5 years ago by mikeperry

See also #9091, which is a related issue/possible dup.

comment:7 Changed 5 years ago by mikeperry

Status: needs_reviewneeds_revision

Ok, I merged arlolra's patch because it is an improvement, but we also want to patch Firefox directly to handle the case where Tor Browser is launched to handle urls, and to ensure that if it does end up docked still, it launches with the right profile and homedir.

comment:8 Changed 5 years ago by mcs

Cc: brade added
Owner: changed from mikeperry to mcs
Status: needs_revisionassigned

Taking ownership of this bug. Kathy Brade and I have a Firefox patch that we will post shortly that modifies the default location for user data (including profiles) to be a directory that is relative to the browser itself. With the patch, docking and launching via desktop URL shortcuts works on both Windows and Mac OS. The launcher programs (e.g., RelativeLink) will not need to pass a profile location; we can just include a profiles.ini file that causes the Tor Browser to use the bundled profile by default. In conjunction with packaging changes planned as part of #9114, this should be a nice improvement.

Changed 5 years ago by mcs

proposed fix (change Firefox user data location)

comment:9 Changed 5 years ago by mikeperry

Keywords: MikePerry201308 added; MikePerry201307 removed

comment:10 Changed 5 years ago by mikeperry

Status: assignedneeds_review

comment:11 Changed 5 years ago by mikeperry

Keywords: MikePerry201308 removed

comment:12 Changed 5 years ago by mikeperry

Status: needs_reviewneeds_information

Ok. I have merged this patch. I will also be merging #9114 soon. If we still need a profiles.ini with both of these merged, can you attach one? (Or one for each platform?)

comment:13 in reply to:  12 Changed 5 years ago by mcs

Replying to mikeperry:

Ok. I have merged this patch. I will also be merging #9114 soon. If we still need a profiles.ini with both of these merged, can you attach one? (Or one for each platform?)

Thanks for the merge. The 9114 patch includes the profiles.ini files:

Bundle-Data/linux/Data/Browser/profiles.ini
Bundle-Data/mac/Data/Browser/profiles.ini
Bundle-Data/windows/Data/Browser/profiles.ini

There is one file for each platform because of the structure under Bundle-Data, but I think all three files have the same content.

comment:14 Changed 5 years ago by mikeperry

Oh, one thing was bothering me about this patch. In nsXREDirProvider::AppendProfilePath(), you removed some initializations that made me nervous. I think in all cases where the empty strings were used, IsEmpty() was checked first, but I removed the code that tries to use them anyways just to be safe:
https://gitweb.torproject.org/tor-browser.git/commitdiff/f0519e332aa292b6b481dec6f9e6a22640157131

Let me know if you think that is a bad idea for some other reason.

comment:15 in reply to:  14 Changed 5 years ago by brade

Replying to mikeperry:

Oh, one thing was bothering me about this patch. In nsXREDirProvider::AppendProfilePath(), you removed some initializations that made me nervous. I think in all cases where the empty strings were used, IsEmpty() was checked first, but I removed the code that tries to use them anyways just to be safe:
https://gitweb.torproject.org/tor-browser.git/commitdiff/f0519e332aa292b6b481dec6f9e6a22640157131

Let me know if you think that is a bad idea for some other reason.

We left the lines in because the patch was smaller (which may reduce future conflicts when merging with future ESR releases).

Your changes look fine, except I would initialize rv to NS_OK rather than an error. If somehow the profile string was empty, the method would fail on the NS_ENSURE_SUCCESS line.

comment:16 Changed 5 years ago by mcs

We committed a follow up fix to address a "profile not found" issue that Mike ran into:

https://gitweb.torproject.org/user/brade/tor-browser.git/commit/0cfceec079329cfedff3baf9f681cfd5860b428d

comment:17 Changed 5 years ago by mcs

Status: needs_informationneeds_review

comment:18 Changed 5 years ago by mcs

Resolution: fixed
Status: needs_reviewclosed

Mike merged these patches into https://gitweb.torproject.org/tor-browser.git
Resolving as fixed.

comment:19 Changed 5 years ago by mcs

Resolution: fixed
Status: closedreopened

Kathy Brade and I discovered that if the browser is started from a shell after cd'ing down into the TBB directory structure, a "." component may be included in the binary path at a low enough point to confuse the code we added that crawls up the directory hierarchy. The result is that the browser will look for its profiles in the wrong directory. Here is a fix:

https://gitweb.torproject.org/user/brade/tor-browser.git/commit/df38389ed9ef4dd8545416eae6ecd155d9b2189c

I suggest just taking this fix for ESR24-based bundles (e.g., TBB 3.5), but it could be back-ported to the FF17-based bundles if people think it is important.

comment:20 Changed 5 years ago by mikeperry

Keywords: MikePerry201311R added
Status: reopenedneeds_review

comment:21 Changed 5 years ago by mikeperry

Keywords: tbb-3.0-backport added
Resolution: fixed
Status: needs_reviewclosed

Ok, I pushed this fixup to the FF24 branch. Can you give a specific repro case? Do you have to do something like .././start-tor-browser.sh? Or something else?

comment:22 in reply to:  21 Changed 5 years ago by mcs

Replying to mikeperry:

Ok, I pushed this fixup to the FF24 branch. Can you give a specific repro case? Do you have to do something like .././start-tor-browser.sh? Or something else?

Thanks for taking this fix. On Mac OS, doing something like this caused the problem:

cd /Users/brade/Desktop/TBB3.0rc1/Contents/MacOS/TorBrowser.app
./Contents/MacOS/firefox

Inside the browser, the "app dir" would then be:

/Users/brade/Desktop/TBB3.0rc1/Contents/MacOS/TorBrowser.app/./Contents/MacOS

Without this fix, the browser would use the wrong profile dir location, which means it tries to create a new profile, which means you end up without tor, and user confusion sets in.

Basically, you need to be lower in the dir hierarchy than usual and use ./ in the command that starts firefox.

comment:23 Changed 6 months ago by arthuredelstein

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