Opened 3 years ago

Closed 3 years ago

#19706 closed defect (fixed)

GetUserDataDirectoryHome in nsXREDirProvider.cpp breaks Orfox

Reported by: amoghbl1 Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-mobile, TorBrowserTeam201608R
Cc: mcs, brade, gk, arthuredelstein Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The GetUserDataDirectoryHome seems to break the page loading in Orfox.
I'm going to attach the patch that I have for this to this ticket, but I think it could be cleaner, I'm not sure what you would require though.

Child Tickets

Attachments (1)

nsXREDirProvider.patch (994 bytes) - added by amoghbl1 3 years ago.

Download all attachments as: .zip

Change History (15)

Changed 3 years ago by amoghbl1

Attachment: nsXREDirProvider.patch added

comment:1 Changed 3 years ago by gk

Keywords: TorBrowserTeam201607R added
Status: newneeds_review

comment:2 Changed 3 years ago by gk

Amogh says this is #9173 related.

comment:3 Changed 3 years ago by mcs

Keywords: TorBrowserTeam201607 added; TorBrowserTeam201607R removed
Status: needs_reviewneeds_revision

Thanks for tracking this down and for providing a patch.

It looks like Orfox stores the browser profile data in the user's home directory (or wherever $HOME points to). If that is the case, then in the long run Orfox should be built with --enable-tor-browser-data-outside-app-dir (which causes TOR_BROWSER_DATA_OUTSIDE_APP_DIR to be defined; see #13252). Unfortunately, the code we added for #13252 has only been tested on OSX so you may not want to use --enable-tor-browser-data-outside-app-dir yet.

Regardless of whether Orfox uses --enable-tor-browser-data-outside-app-dir or not, Kathy and I would like to use a revised patch that modifies TorBrowser_GetUserDataDir() (which is inside xpcom/io/TorFileUtils.cpp). We may also need to make a small patch to nsXREDirProvider::GetUserDataDirectoryHome() to avoid putting the data underneath a Data/Browser subdirectory within $HOME.

Do you want Kathy and me to provide an untested patch for you to try, or do you want to create a revised patch?

For example, if you do not build with --enable-tor-browser-data-outside-app-dir, you would want to insert a #elif defined(ANDROID) block before the following code in TorBrowser_GetUserDataDir():

#else
  // User data is embedded within the application directory (i.e.,
  // TOR_BROWSER_DATA_OUTSIDE_APP_DIR is not defined).
  nsresult rv = GetAppRootDir(aExeFile, getter_AddRefs(tbDataDir));
  NS_ENSURE_SUCCESS(rv, rv);
  rv = tbDataDir->AppendNative(NS_LITERAL_CSTRING("TorBrowser"));
  NS_ENSURE_SUCCESS(rv, rv);
#endif

comment:4 Changed 3 years ago by amoghbl1

We don't mind building with or without the --enable-tor-browser-data-outside-app-dir flag.

I think I will let you get this patched as I'm not sure what the best way to deal with it is.

I don't think that we require a patch right now, as I'm fine with using this one for builds we're trying to get out now. But it would be useful to have this fixed by the next time we do a rebase.

comment:5 Changed 3 years ago by n8fr8

In the case of Android, each app is a user itself, and so the home is a path unique for the app, and that only the app can read and write to. There is no other way or place for the data to be safely stored.

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

Status: needs_revisionneeds_information

Replying to amoghbl1:

I don't think that we require a patch right now, as I'm fine with using this one for builds we're trying to get out now. But it would be useful to have this fixed by the next time we do a rebase.

I am concerned that we will forget about this if we do not address it now. If Kathy and I create a patch, do you have time to try it (compile it and let us know if it works)?

comment:7 Changed 3 years ago by gk

Status: needs_informationassigned

Yes, we should write a patch for that now-ish. We already forgot this once.

comment:8 in reply to:  6 Changed 3 years ago by amoghbl1

Replying to mcs:

Replying to amoghbl1:

I don't think that we require a patch right now, as I'm fine with using this one for builds we're trying to get out now. But it would be useful to have this fixed by the next time we do a rebase.

I am concerned that we will forget about this if we do not address it now. If Kathy and I create a patch, do you have time to try it (compile it and let us know if it works)?

Sure, I will test it out once it's ready and verify whether it works or not.

comment:9 Changed 3 years ago by mcs

Keywords: TorBrowserTeam201607R added; TorBrowserTeam201607 removed
Status: assignedneeds_review

comment:10 in reply to:  9 Changed 3 years ago by amoghbl1

Replying to mcs:

Please try the following patch:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug19706-01&id=d86fa0e328b8ea6a07741b55da5cc1bebb669b88

This patch works fine with our build. Thank you for it!

comment:11 Changed 3 years ago by gk

Keywords: TorBrowserTeam201608R added; TorBrowserTeam201607R removed

Old reviews for the "new" month.

comment:12 Changed 3 years ago by gk

Cc: arthuredelstein added

Looks good to me. Arhur, could you have a look at it as well?

comment:13 Changed 3 years ago by arthuredelstein

Also looks good to me. Sorry for the delay!

comment:14 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Fixed on tor-browser-45.3.0esr-6.0-1 (commit 7c50120b217807b98a8a99d16c53b760d2119280) and tor-browser-45.3.0esr-6.5-1 (commit b8726ee81e6eca2e302e2ea85c0d09ad22be1ec5).

Note: See TracTickets for help on using tickets.