Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#15564 closed defect (fixed)

Isolate SharedWorker by first party domain

Reported by: arthuredelstein Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-linkability, tbb-usability-website, TorBrowserTeam201512R
Cc: mcs, gk Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We may be able to write a C++ patch that isolates SharedWorker's by first party domain. For now, we are probably going to disable SharedWorkers by a pref.

Child Tickets

Change History (14)

comment:1 Changed 5 years ago by mcs

Cc: mcs added

comment:2 Changed 5 years ago by mikeperry

Keywords: tbb-usability-website added

comment:3 Changed 5 years ago by gk

Cc: gk added

comment:4 Changed 4 years ago by mikeperry

SharedWorkers also expose a different implementation of the performance timers and high-res clock source (https://developer.mozilla.org/en-US/docs/Web/API/WorkerGlobalScope/performance). We'll need to apply the #1517 changes to those, too.

comment:5 in reply to:  4 Changed 4 years ago by gk

Replying to mikeperry:

SharedWorkers also expose a different implementation of the performance timers and high-res clock source (https://developer.mozilla.org/en-US/docs/Web/API/WorkerGlobalScope/performance). We'll need to apply the #1517 changes to those, too.

See: #16340 and #16110 for this issue.

comment:6 Changed 4 years ago by arthuredelstein

Keywords: TorBrowserTeam201510R added
Status: newneeds_review

Here's a series of patches that re-enable SharedWorkers and isolate them by first party domain. Note that there are 3 commits that need to be applied:

https://github.com/arthuredelstein/tor-browser/commits/15564+1

comment:7 Changed 4 years ago by gk

Keywords: TorBrowserTeam201511R added; TorBrowserTeam201510R removed

comment:8 Changed 4 years ago by mikeperry

Keywords: TorBrowserTeam201512R added; TorBrowserTeam201511R removed

comment:9 Changed 4 years ago by mcs

Severity: Normal
Status: needs_reviewneeds_revision

Kathy and I reviewed this and the code looks correct. Nice work! Of course we have a few comments:

  • Since the dom.workers.sharedWorkers.enabled pref is true by default, we probably should just remove it from 000-tor-browser.js.
  • Inside dom/base/test/test_tor_bug15564.html near setPref there is a typo in a comment: "prmoise" should be "promise".
  • Is it customary to close tabs in test code or not? Kathy and I did so in docshell/test/test_tor_bug16620.html but you did not here. I can see that leaving them open would be helpful for debugging individual test cases but I do not know what the norm is for Mozilla test code.
  • It took us a little while to understand the flow of messages in the test code. Maybe add a few comments to help new readers.

comment:10 in reply to:  9 Changed 4 years ago by arthuredelstein

Status: needs_revisionneeds_review

Replying to mcs:

Kathy and I reviewed this and the code looks correct. Nice work! Of course we have a few comments:

  • Since the dom.workers.sharedWorkers.enabled pref is true by default, we probably should just remove it from 000-tor-browser.js.
  • Inside dom/base/test/test_tor_bug15564.html near setPref there is a typo in a comment: "prmoise" should be "promise".
  • Is it customary to close tabs in test code or not? Kathy and I did so in docshell/test/test_tor_bug16620.html but you did not here. I can see that leaving them open would be helpful for debugging individual test cases but I do not know what the norm is for Mozilla test code.
  • It took us a little while to understand the flow of messages in the test code. Maybe add a few comments to help new readers.

Thanks for the review! I've adopted all of your suggestions and rebased to the latest tor-browser 5.5a branch:

https://github.com/arthuredelstein/tor-browser/commits/15564+3

comment:11 Changed 4 years ago by mcs

r=brade, r=mcs
You went "above and beyond" with the comments, but they should help future readers of the test code (and any of us who might look later to steal ideas from your code ;)

comment:12 Changed 4 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Thanks, this looks good to me (nice test!). I did not test it due to lack of time. We'll see if it explodes while building the alpha. :) I made a fixup commit to correct some typos in the main test. I left "domains methods" (in the test) although I am unsure what that means to be honest.

comment:13 in reply to:  12 Changed 4 years ago by arthuredelstein

Replying to gk:

Thanks, this looks good to me (nice test!). I did not test it due to lack of time. We'll see if it explodes while building the alpha. :) I made a fixup commit to correct some typos in the main test. I left "domains methods" (in the test) although I am unsure what that means to be honest.

Oops, the word "methods" shouldn't be there at all. Here's a fixup:
https://github.com/arthuredelstein/tor-browser/commit/4ff52d2e7b7c36f524660a9f5b7f083ab49caba3

comment:14 Changed 4 years ago by gk

Applied to tor-browser-38.5.0esr-5.5-1 (commit 07c085d76a70e9bb0ed81fe810936f7f2f659389).

Note: See TracTickets for help on using tickets.