Windows Jump Lists fail for Tor Browser

This ticket is a spinoff of #6062 (moved) (which I am going to close).

Windows 7 and 8 include a jump lists feature which is used by Firefox to provide a menu of tasks that may be accessed from the Start Menu or Taskbar ("Open new tab", "Open new window", etc.) These task items fail in Tor Browser because we have disabled remoting. Similarly, clicking the main "Tor Browser" pinned item fails for the same reason if Tor Browser is already running.

For now, we should change things so we do not show the jump list items. We can do so by setting these two prefs to false:

browser.taskbar.lists.frequent.enabled browser.taskbar.lists.tasks.enabled

(we also want browser.taskbar.lists.recent.enabled = false but that is already done by default).

Also – although one would think that setting browser.taskbar.lists.enabled = false would be sufficient to turn everything off, doing that may leave old jump list menu items around. So it is better to turn off the three more specific prefs. See:

http://mxr.mozilla.org/mozilla-esr24/source/browser/modules/WindowsJumpLists.jsm#219

added TorBrowserTeam201901R component::applications/tor browser owner::pospeselr priority::medium resolution::fixed severity::normal status::closed tbb-usability type::defect labels

Trac:
Cc: brade to brade, gk

Related: https://trac.torproject.org/projects/tor/ticket/5203#comment:5

#11105 (moved) is a duplicate.

Trac:
Reviewer: N/A to N/A
Keywords: N/A deleted, tbb-usability added
Sponsor: N/A to N/A
Severity: N/A to Normal

Trac:
Cc: brade, gk to brade, gk, user1128

#28542 (moved) is a duplicate.

Trac:
Owner: mcs to pospeselr
Status: new to assigned
Cc: brade, gk, user1128 to brade, gk, user1128, tbb-team

tor-browser patch implementing the options described in the ticket. Verified that a windows build no longer includes the jumplist entries.

https://gitweb.torproject.org/user/richard/tor-browser.git/commit/?h=bug_12885

Trac:
Keywords: N/A deleted, TorBrowserTeam201812R added
Status: assigned to needs_review

Your patch looks OK, but how does the browser.taskbar.lists.enabled pref relate to the ones you are setting to false? It looks like it might be a "master switch" that could be used to turn everything off, but I would need to look more closely at the code to be sure.

Replying to mcs:

Your patch looks OK, but how does the browser.taskbar.lists.enabled pref relate to the ones you are setting to false? It looks like it might be a "master switch" that could be used to turn everything off, but I would need to look more closely at the code to be sure.

Based on some experimentation, the browser.taskbar.lists.enabled pref does not appear to act as a master switch in this way. After reading the code it looks like it may clear the 'recents' list when toggled from true to false, but that's about it.

Replying to pospeselr:

Replying to mcs:

Your patch looks OK, but how does the browser.taskbar.lists.enabled pref relate to the ones you are setting to false? It looks like it might be a "master switch" that could be used to turn everything off, but I would need to look more closely at the code to be sure.

Based on some experimentation, the browser.taskbar.lists.enabled pref does not appear to act as a master switch in this way. After reading the code it looks like it may clear the 'recents' list when toggled from true to false, but that's about it.

So, in fact, this does look like a (potential) master switch, the problem is that old entries are not deleted. It seems a bit weird to me that we need to have the whole thing enabled (with the timer to update the jumplist still firing all the time) to be sure old entries are deleted and no new ones are created.

What about patching the code instead to make sure once during start-up, in case browser.taskbar.lists.enabled is set to false, we remove old items, too. The entry point seems to be

https://dxr.mozilla.org/mozilla-esr60/source/browser/components/nsBrowserGlue.js#1173 calling temp.WinTaskbarJumpList.startup();. That in turn makes us read our prefs (this._refreshPrefs();) and start the timer for the jump list update (this._updateTimer();). Maybe we should just call the timer update in case the jumplist feature is enabled and if not we delete recent entries and that's it? (there would then be no need to disable the three prefs, rather we would have turned browser.taskbar.lists.enabled into the global switch which its name suggests.

Does that sound reasonable? Or did I miss something while looking at the code?

Trac:
Status: needs_review to needs_information

Clearing review state for now to get more info.

Trac:
Keywords: TorBrowserTeam201812R deleted, TorBrowserTeam201812 added

EDIT: ignore this patch it's no longer relevant skip down here: https://trac.torproject.org/projects/tor/ticket/12885?cnum_edit=11#comment:13

Updated patch now explicitly checking for the 'active' pref and deleting an existing jumplist if so: https://gitweb.torproject.org/user/richard/tor-browser.git/diff/?h=bug_12885_v2&id2=8bc2ca14f51f4b57efcadd6f7405e0a4d48f2204&context=8&ignorews=0&dt=0

This patch should also partially fix the disk leak associated with Jump Lists; jump list entries are stored in a binary file in %APPDATA%\Microsoft\Windows\Recent\CustomDestinations which contain identifying strings. It would seem the existing _deleteActiveJumpList() function at least 'clears' the file, and the filename of the store is dependent on install location (so there's not a globally unique Tor Browser jumplist store file you can look for).

Trac:
Keywords: TorBrowserTeam201812 deleted, TorBrowserTeam201901R added
Status: needs_information to needs_review

Updated patch: https://gitweb.torproject.org/user/richard/tor-browser.git/commit/?h=bug_12885_v3

Length explanation in the commit message, but the tldr is that versions of Tor Browser with this patch applied will have Jump Lists disabled and will clean up the backing store if users go in and toggle them on and off via about:config settings. However, all existing versions of Tor Browser for Windows create a file in %APPDATA%\Microsoft\Windows\Recent\CustomDestinations that contains the path to the Tor Browser install that we can't 'cleanly' (ie via the Microsoft-provided Jump List APIs). A separate patch could go in and purge files in that directory containing 'Tor Browser' strings but that seems like a good idea for another ticket.

EDIT: Created ticket #28996 (moved) to handle disk-cleanup

So, _buildlist(), where you check !this._enabled, is only called from update() as far as I can see it. But that function is already checking !this._enabled and returning in that case before _buildlist() is called. How does one reach your introduced check?

What speaks against the minimal idea I had in comment:9?

Trac:
Status: needs_review to needs_information

from IRC:

---

(2019-01-24 2:03:34 PM) pospeselr: GeKo: ok fun fact, the code change isn't even necessary for #12885 (moved) (2019-01-24 2:04:14 PM) pospeselr: all we need to do is flip taskbar.grouping.useprofile to true and all of the browser.taskbar.lists.* options work as expected (2019-01-24 2:04:16 PM) GeKo: which? (2019-01-24 2:04:25 PM) GeKo: ha, okay (2019-01-24 2:04:35 PM) pospeselr: including browser.taskbar.lists.enabled acting as a master switch (2019-01-24 2:05:10 PM) pospeselr: just verified stock (2019-01-24 2:05:14 PM) pospeselr: on stock* (2019-01-24 2:07:03 PM) GeKo: yay (2019-01-24 2:09:03 PM) pospeselr: i'll update the patch and strip out the code change

Amended the change to remove the unnecessary code change, left the prefs change.

tor-browser: https://gitweb.torproject.org/user/richard/tor-browser.git/commit/?h=bug_12885_v3

Trac:
Status: needs_information to needs_review

Okay, this is commit f4606d1c1c35eb36edf3c7cd6b2904be01f19f32 on tor-browser-60.4.0esr-8.5-1 now. I think we are good here.

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

closed

mentioned in issue #18166 (moved)

mentioned in issue #19414 (moved)

mentioned in issue #28542 (moved)

moved to tpo/applications/tor-browser#12885 (closed)