Opened 5 months ago

Closed 12 days ago

Last modified 11 days ago

#30504 closed task (fixed)

Investigate if New Identity works properly after moving to ESR 68

Reported by: acat Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: ff68-esr, tbb-newnym, tbb-9.0-must-alpha, TorBrowserTeam201910R
Cc: Actual Points: 1.5
Parent ID: Points: 0.25
Reviewer: Sponsor: Sponsor44-can

Description (last modified by acat)

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

Child Tickets

Change History (19)

comment:1 Changed 5 months ago by acat

Keywords: TorBrowserTeam201905 ff68-esr added

comment:2 Changed 5 months ago by acat

Description: modified (diff)

comment:3 Changed 5 months ago by acat

Keywords: tbb-newnym added; TorBrowserTeam201905 removed

comment:4 Changed 3 months ago by pili

Sponsor: Sponsor44-can

Adding Sponsor 44 to ESR68 tickets

comment:5 Changed 7 weeks ago by gk

Keywords: TorBrowserTeam201909 tbb-9.0-must-alpha added

Let's double-check for realz.

comment:6 Changed 7 weeks ago by pili

Points: 0.25

comment:7 Changed 3 weeks ago by acat

Actual Points: 1.5
Keywords: TorBrowserTeam201909R added; TorBrowserTeam201909 removed
Status: newneeds_review

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

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", "");.

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

Last edited 2 weeks ago by gk (previous) (diff)

comment:8 Changed 3 weeks ago by cypherpunks

Looks promising! Needs tests.

comment:9 Changed 3 weeks ago by pili

Keywords: TorBrowserTeam201910R added; TorBrowserTeam201909R removed

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

comment:10 Changed 3 weeks ago by acat

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.

comment:11 Changed 2 weeks ago by acat

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.

comment:12 in reply to:  7 ; Changed 2 weeks ago by gk

Status: needs_reviewneeds_revision

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, "browser: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. 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_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?

comment:13 Changed 2 weeks ago by gk

Keywords: TorBrowserTeam201910 added; TorBrowserTeam201910R removed

comment:14 in reply to:  12 ; Changed 13 days ago by acat

Keywords: TorBrowserTeam201910R added; TorBrowserTeam201910 removed
Status: needs_revisionneeds_review

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

comment:15 Changed 12 days ago by acat

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

comment:17 Changed 12 days ago by acat

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

comment:18 Changed 12 days ago by gk

Resolution: fixed
Status: needs_reviewclosed

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

comment:19 in reply to:  14 Changed 11 days ago by gk

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

Note: See TracTickets for help on using tickets.