Opened 5 years ago

Closed 7 months ago

#12885 closed defect (fixed)

Windows Jump Lists fail for Tor Browser

Reported by: mcs Owned by: pospeselr
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-usability, TorBrowserTeam201901R
Cc: brade, gk, user1128, tbb-team Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

This ticket is a spinoff of #6062 (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

Child Tickets

Change History (16)

comment:1 Changed 5 years ago by gk

Cc: gk added

comment:3 Changed 3 years ago by bugzilla

Keywords: tbb-usability added
Severity: Normal

#11105 is a duplicate.

comment:4 Changed 3 years ago by gk

Cc: user1128 added

comment:5 Changed 9 months ago by gk

Cc: tbb-team added
Owner: changed from mcs to pospeselr
Status: newassigned

#28542 is a duplicate.

comment:6 Changed 9 months ago by pospeselr

Keywords: TorBrowserTeam201812R added
Status: assignedneeds_review

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

comment:7 Changed 8 months ago by 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.

comment:8 in reply to:  7 ; Changed 8 months ago by 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.

comment:9 in reply to:  8 Changed 8 months ago by gk

Status: needs_reviewneeds_information

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?

comment:10 Changed 8 months ago by gk

Keywords: TorBrowserTeam201812 added; TorBrowserTeam201812R removed

Clearing review state for now to get more info.

comment:11 Changed 8 months ago by pospeselr

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

Last edited 8 months ago by pospeselr (previous) (diff)

comment:12 Changed 8 months ago by pospeselr

Keywords: TorBrowserTeam201901R added; TorBrowserTeam201812 removed
Status: needs_informationneeds_review

comment:13 Changed 8 months ago by pospeselr

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.

Version 0, edited 8 months ago by pospeselr (next)

comment:14 Changed 7 months ago by gk

Status: needs_reviewneeds_information

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?

comment:15 Changed 7 months ago by pospeselr

Status: needs_informationneeds_review

from IRC:


(2019-01-24 2:03:34 PM) pospeselr: GeKo: ok fun fact, the code change isn't even necessary for #12885
(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

Last edited 7 months ago by pospeselr (previous) (diff)

comment:16 Changed 7 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

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

Note: See TracTickets for help on using tickets.