Investigate if New Identity works properly after moving to ESR 68

added BugSmashFund TorBrowserTeam201910R actualpoints::1.5 component::applications/tor browser ff68-esr owner::tbb-team points::0.25 priority::medium resolution::fixed severity::normal status::closed tbb-9.0-must-alpha tbb-newnym type::task labels

Trac:
Keywords: N/A deleted, TorBrowserTeam201905 ff68-esr added

Trac:
Description: Some early tests are showing a few errors and warnings in Browser Console:

PushService: onPermissionChange: Error updating registrations: InvalidStateError PushService.jsm:302

An IndexedDB transaction that was not yet complete has been aborted due to page navigation. IndexedDBHelper.jsm:145:23

Error: _initWorker called too early! Please read the session file from disk first. SessionFile.jsm:334:15

TypeError: win.gBrowser is undefined ProcessHangMonitor.jsm:410:18

to

Apparently, it seems to be working. But after reloading Browser Console shows a few errors and warnings:

PushService: onPermissionChange: Error updating registrations: InvalidStateError PushService.jsm:302

An IndexedDB transaction that was not yet complete has been aborted due to page navigation. IndexedDBHelper.jsm:145:23

Error: _initWorker called too early! Please read the session file from disk first. SessionFile.jsm:334:15

TypeError: win.gBrowser is undefined ProcessHangMonitor.jsm:410:18

Trac:
Keywords: TorBrowserTeam201905 ff68-esr deleted, ff68-esr tbb-newnym added

Adding Sponsor 44 to ESR68 tickets

Trac:
Sponsor: N/A to Sponsor44-can

Let's double-check for realz.

Trac:
Keywords: N/A deleted, tbb-9.0-must-alpha, TorBrowserTeam201909 added

Trac:
Points: N/A to 0.25

Changes in https://github.com/acatarineu/torbutton/commit/30504.

PushService: onPermissionChange: Error updating registrations: InvalidStateError PushService.jsm:302

I can't see this error anymore.

An IndexedDB transaction that was not yet complete has been aborted due to page navigation. IndexedDBHelper.jsm:145:23

This was due to Services.qms.clear() interfering with internal open indexeddbs, which was partially fixed in #31396 (moved) and with that this error is not present anymore. In any case, I replaced Services.qms.clear by something that does not erase internal browser storage, but just content (using Services.clearData and the CLEAR_DOM_STORAGES flag which is CLEAR_APPCACHE | CLEAR_DOM_QUOTA | CLEAR_DOM_PUSH_NOTIFICATIONS | CLEAR_REPORTS (see https://searchfox.org/mozilla-esr68/rev/65b2bc1788c28cf97933c198e3e6bff3817f2d86/toolkit/components/cleardata/ClearDataService.jsm). I also removed the Services.qms.clear() on startup, since it should no longer be needed for removing asm.js caches (#19417 (moved)).

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, "[purge-session-history",](purge-session-history",) "");.

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.

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 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.

Trac:
Actualpoints: N/A to 1.5
Status: new to needs_review
Keywords: TorBrowserTeam201909 deleted, TorBrowserTeam201909R added

Looks promising! Needs tests.

We're now in October, moving September outstanding reviews to October

Trac:
Keywords: TorBrowserTeam201909R, ff68-esr tbb-newnym deleted, tbb-newnym, TorBrowserTeam201910R, ff68-esr added

If the Services.clearData(CLEAR_DOM_STORAGES) change is fine, we could also revert the extensions.webextensions.ExtensionStorageIDB.enabled pref change that we had to do because of this.

https://www.github.com/acatarineu/torbutton/commit/30504+1.

Leaving the torbutton_init Services.qms.clear() as discussed on IRC, it's not really related to this issue and will be addressed in #19417 (moved).

Replying to 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, "[purge-session-history",](purge-session-history",) "");.

Yes, filing a ticket here sounds good, please do.

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 (moved). 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.

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 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?

Trac:
Status: needs_review to needs_revision

Trac:
Keywords: TorBrowserTeam201910R deleted, TorBrowserTeam201910 added

Replying to gk:

Replying to 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, "[purge-session-history",](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 (moved). 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 (moved).

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(() = {...});...).

Trac:
Keywords: TorBrowserTeam201910 deleted, TorBrowserTeam201910R added
Status: needs_revision to needs_review

Fixup for switching back to idb storage for webext: https://github.com/acatarineut/tor-browser/commit/30504.

I think it's not possible to do a fixup/squash and modify the original commit message, right? Because the preferences commit message includes #31396 (moved) and ideally we would want to remove that.

Besides, this fixup is a bit tricky, since we would also want to revert all the extensions.webextensions.ExtensionStorageIDB.migrated pref branch so that the idb storage migration is performed again for all extensions. This is because it might be possible that this migration was done, then "destroyed" by the previous Services.qms.clear() and switching this back leads to a bad state.

So perhaps it's better to open a new ticket for this and do it later...

Sorry, link was wrong: https://github.com/acatarineu/tor-browser/commit/30504.

Created a ticket for the idb storage issue: #32017 (moved). So we should be good for now without the tor-browser fixup.

Thanks. Cherry-picked the Torbutton patch to master (commit 6c8a666632ec6b838f7dd724f4fe0b364248b01b).

Trac:
Status: needs_review to closed
Resolution: N/A to fixed

Replying to acat:

Replying to gk:

Replying to acat:

[snip]

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 (moved). 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).

Yes, that's true. But with your code the window would jump around even if you not move it which is not currently happening (at least not on the machine I tested).

[snip]