[tor-bugs] #28822 [Applications/Tor Browser]: re-implement desktop onboarding for ESR 68

Wed Sep 4 16:42:15 UTC 2019

#28822: re-implement desktop onboarding for ESR 68
---------------------------------------------+-----------------------------
 Reporter:  mcs                              |          Owner:  tbb-team
     Type:  defect                           |         Status:
                                             |  needs_review
 Priority:  Medium                           |      Milestone:
Component:  Applications/Tor Browser         |        Version:
 Severity:  Normal                           |     Resolution:
 Keywords:  ff68-esr, TorBrowserTeam201909R  |  Actual Points:
Parent ID:  #30429                           |         Points:  4
 Reviewer:                                   |        Sponsor:
                                             |  Sponsor44-can
---------------------------------------------+-----------------------------
Changes (by acat):

 * status:  needs_revision => needs_review
 * keywords:  ff68-esr, TorBrowserTeam201909 => ff68-esr,
     TorBrowserTeam201909R

Comment:

 Addressed comments in https://github.com/acatarineu/torbutton/commit/28822
 and https://github.com/acatarineu/tor-browser/commits/28822+1 (last 5
 commits).

 Replying to [comment:6 gk]:

 > Okay, I took the commits from your `30429+9` and here are my comments:
 >
 > ea5f8c2c9ccc0130944f05a6e31ebb2a9322041c - I think it's mostly good. Two
 things:
 > {{{
 > +const {Services}
 > }}}
 > I think we settled for space between name and braces in Torbutton? Would
 be good
 > here as well.

 Fixed.

 > {{{
 > +        @$(MAKE) -C ../extensions/onboarding/locales chrome AB_CD=$*
 > }}}
 > It's really just one level up here, right? While all the other items are
 two
 > levels up?

 We could change it to

 {{{
 +       @$(MAKE) -C ../../browser/extensions/onboarding/locales chrome
 AB_CD=$*
 }}}
 but I think it's the same as the current one, since the file is already in
 browser `browser/locales/Makefile.in`.

 > 1c9eb3993c5b505c0894b13634b09690bfb97791 - not okay (not sure about the
 changed `onboarding-overlay-button` but we'll see I guess while testing)

 I changed the css to match the old patch, since the `onboarding-overlay-
 attention-dot` was not showing properly.

 > The images are the wrong ones. We want to have those from #30560.

 Fixed.

 > 6f05a139b387c072a63bfae3a086aee2cee95875 - okay
 > e19e128476f48278911656db735739f0526f12ce - not okay
 > {{{
 > -/* The primary button gets the same color as the customize button. */
 > }}}
 > in `browser/themes/shared/UITour.inc.css` is missing

 Fixed.

 > 742fccfcbb7a19ba9daee44335e9962639773d13 - not okay
 > {{{
 > -  OnboardingTelemetry:
 "resource://onboarding/modules/OnboardingTelemetry.jsm",
 > }}}
 > in `browser/extensions/onboarding/bootstrap.js` is missing

 Fixed.

 > aeb0b6678e61fd282825610ca29a225eb0991281 - I think this is okay, but are
 we affected by https://bugzilla.mozilla.org/show_bug.cgi?id=1498378?

 We are, nice catch. Reverted that one too.

 > Additionally, upon further testing it seems to me that the details part
 of the circuit level and the security settings is not working as expected.
 While clicking on the former opens the DDG .onion, no tour through the
 display shows up trying to make the menu behind the security settings
 button visible by clicking on the option does not do anything either.

 It seems there were several recent patches which did not like the
 onboarding. The security settings not opening is a regression due to
 #31322 and then #31251. The first one prevents UITour-lib.js from loading,
 and the second one makes `window.document.getElementById("security-level-
 button").doCommand()` not open the security level anymore (not sure why).

 The circuit path problem seems to be a regression caused by the FPI
 permissions patches. I tried reverting `Bug 1330467 - part 1. Don't strip
 first party domain from permissions key; r=johannh,Ehsan` and it works. It
 also works if you disable `firstparty.isolate`.

 The problem is that the permission check in [https://searchfox.org
 /mozilla-
 central/rev/9bb55ae4d808fc48afcf93f99da6a685265b86c6/browser/components/uitour/UITourChild.jsm#91
 UITourChild.jsm] returns `false` after that patch is applied. I think this
 is because the `uitour` permission for the DDG onion page is not being
 loaded in the content process (where this is called). These are loaded in
 [https://searchfox.org/mozilla-
 central/rev/9bb55ae4d808fc48afcf93f99da6a685265b86c6/dom/ipc/ContentParent.cpp#5450
 ContentParent.cpp] (for the principal). Before the FPI permission patch
 (or when disabling `firstparty.isolate`, this is called with key
 `https://3g2upl4pq6kufc4m.onion`, which loads the permission properly.
 After the patch it's called with key
 `https://3g2upl4pq6kufc4m.onion^firstPartyDomain=3g2upl4pq6kufc4m.onion`,
 which apparently does not work. This issue does not occur with `about:tor`
 `uitour` permission: this is always loaded because
 `nsPermissionManager::GetKeyForOrigin` returns empty keys for origins not
 starting with `http://`, `https://` or `ftp://`. The convention is that
 permissions with empty keys are default permissions and always preloaded
 (see [https://searchfox.org/mozilla-
 central/rev/9bb55ae4d808fc48afcf93f99da6a685265b86c6/dom/ipc/ContentParent.cpp#1340
 ContentParent.cpp]). So I fixed this by adding `uitour` to the permissions
 that are always preloaded.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/28822#comment:11>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online