[tor-bugs] #30504 [Applications/Tor Browser]: Investigate if New Identity works properly after moving to ESR 68

[Tor Bug Tracker & Wiki]

#30504: Investigate if New Identity works properly after moving to ESR 68
-------------------------------------------------+-------------------------
 Reporter:  acat                                 |          Owner:  tbb-
                                                 |  team
     Type:  task                                 |         Status:
                                                 |  needs_review
 Priority:  Medium                               |      Milestone:
Component:  Applications/Tor Browser             |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  ff68-esr, tbb-newnym, tbb-9.0-must-  |  Actual Points:  1.5
  alpha, TorBrowserTeam201910R                   |
Parent ID:                                       |         Points:  0.25
 Reviewer:                                       |        Sponsor:
                                                 |  Sponsor44-can
-------------------------------------------------+-------------------------
Changes (by acat):

 * status:  needs_revision => needs_review
 * keywords:  ff68-esr, tbb-newnym, tbb-9.0-must-alpha, TorBrowserTeam201910
     => ff68-esr, tbb-newnym, tbb-9.0-must-alpha, TorBrowserTeam201910R


Comment:

 Replying to [comment:12 gk]:
 > Replying to [comment:7 acat]:
 >
 > [snip]
 >
 > > `Error: _initWorker called too early! Please read the session file
 from disk first. SessionFile.jsm:334:15` this one is also in esr60. And
 can also be reproduced in latest Firefox with
 `browser.privatebrowsing.autostart = true` when using the "Forget" feature
 which is "hidden" in `Customize...` menu (so perhaps we could file a
 bugzilla issue for that). This is caused by
 `Services.obs.notifyObservers(null, "browser:purge-session-history",
 "");`.
 >
 > Yes, filing a ticket here sounds good, please do.

 https://bugzilla.mozilla.org/show_bug.cgi?id=1587358

 ----

 > > `TypeError: win.gBrowser is undefined ProcessHangMonitor.jsm:410:18`
 > >
 > > I fixed this one by waiting the new window to be initialized before
 closing the last one.
 >
 > Hrm. The drawback of the fix is that the window is now "jumping" around
 the screen which might not be desired. That's because the code seems to
 not open a new window directly at the place where the current one still is
 which was no problem when we closed the older one first.
 >
 > I feel that change is not worth it. That is a usability issue that got
 reported in the past actually, see #22536. I think it got better with e10s
 improvements as the ticket mentions but we should not make it worse again.
 I wonder whether we can patch the code at a different place instead.

 I just checked this, and it's happening in the nightlies without this
 change, too. For me it opens the new window in a different place than the
 old one (if you move it from the original position).

 In any case, I reverted that change in
 https://github.com/acatarineu/torbutton/commit/30504+2 and for some
 reason, I do not see the `TypeError: win.gBrowser is undefined
 ProcessHangMonitor.jsm:410:18` error anymore. So it must be either some
 race condition or it got fixed by something else...

 I also added a try/catch for the new `clearData`, as done in several other
 steps of new identity.

 ----

 > > BTW, there are several steps of new identity data clearing which might
 be performed via `Services.clearData(SOME_FLAG_COMBINATION)` (which I
 think is new in esr68), but I'm not sure it's a good idea to change those
 right now. In any case, I compared all possible flags with what is
 currently
 >
 > We can do a clean up here after Tor Browser 9, I think. Could you open a
 ticket for that?

 Done in #32012.

 ----

 > > done in new_identity, just in case we might be missing something, and
 found some which are probably not needed but I thought they would not do
 harm if we also include them:
 > >
 > > * `CLEAR_PREDICTOR_NETWORK_DATA`: Should not be needed since network
 predictor is disabled.
 > >
 > > * `CLEAR_PLUGIN_DATA`: One of the components of `Sanitizer.cookies`:
 https://searchfox.org/mozilla-
 esr68/rev/65b2bc1788c28cf97933c198e3e6bff3817f2d86/browser/modules/Sanitizer.jsm#373.
 Again we should not have plugins enabled, but we might want to have it
 still.
 > >
 > > * `CLEAR_MEDIA_DEVICES`: Media devices are disabled
 > >
 > > Note that replacing `Services.qms.clear` by
 `Services.clearData(CLEAR_DOM_STORAGES` also adds some flags that clear a
 couple of things currently not in torbutton:
 > >
 > > * `CLEAR_REPORTS`: Clears CSP reports? I did not investigate how these
 reports are sent currently.
 > >
 > > * `CLEAR_DOM_PUSH_NOTIFICATIONS`: Not sure if this is an issue without
 service workers.
 > >
 > > I think all the rest of flags in `ClearDataService.jsm` are already
 covered in torbutton in one way or another.
 >
 > Thanks for the investigation. Out of curiosity what made you use
 `aysnc`/`await` in some places now?

 The `Services.clearData` API returns a promise, so some operations/flags
 might be async. The await/asyncs are to make sure that we wait for these
 promises in `torbutton_new_identity` before executing the next step, while
 keeping the same code structure (no need to start doing `promise.then(() =
 {...});`...).

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