Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#20043 closed defect (fixed)

SharedWorker uses catchall circuit

Reported by: bugzilla Owned by: tbb-team
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Major Keywords: tbb-linkability, TorBrowserTeam201610R
Cc: arthuredelstein, mcs, brade Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

STR: https://mdn.github.io/simple-shared-worker/

getFirstPartyURI failed for https://mdn.github.io/simple-shared-worker/worker.js: 0x80070057
Torbutton INFO: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [mozIThirdPartyUtil.getFirstPartyURIFromChannel]
Torbutton INFO: tor SOCKS isolation catchall: https://mdn.github.io/simple-shared-worker/worker.js via --unknown--:508fd21f6097f45ba346eaccf8411d0a

(and dom.workers.sharedWorkers.enabled is set to false in TBB 6.5a2 ;)

Child Tickets

Change History (16)

comment:1 Changed 3 years ago by gk

Cc: arthuredelstein added
Keywords: tbb-pref removed
Priority: MediumHigh
Severity: NormalMajor

That pref does not exist anymore in 6.5a2. Not sure what Tor Browser you are using. Maybe it is somehow a left-over? In any case that pref does not do anything anymore.

That said, I can see a similar log output on the command line.

comment:2 Changed 3 years ago by bugzilla

It's not funny at all. What is able to change prefs? Do Workers have more permissions than needed?
TBB is 6.5a2 from 6.5a1 from 6.0a5 (testing in a clean environment = no pref).

Version 1, edited 3 years ago by bugzilla (previous) (next) (diff)

comment:3 Changed 3 years ago by arthuredelstein

Keywords: TorBrowserTeam201609R added
Status: newneeds_review

Thanks for discovering and reporting this issue, bugzilla.

Here are patches for review. There are two commits: a fix and a regression test.

https://github.com/arthuredelstein/tor-browser/commits/20043

When I revert the fix, the regression test fails.

For the record, this problem occurs because SharedWorkers are supposed to be able to outlive their originating top-level document. So https://bugzilla.mozilla.org/show_bug.cgi?id=1118845 correctly stops retaining a pointer to that document in the load request. Unfortunately, that also means we can't rely on having the top-level document to obtain the first-party domain. So we use the backup option (also used in Tor Browser patches for first-party isolation of OCSP, Favicons, and link rel=preconnect) of GetDocumentURI/SetDocumentURI.

comment:4 Changed 3 years ago by gk

Could you check the rv of NS_NewURI()? That would fit better in the code context. Not sure what we want to do if that call failed, though. Could/should we emit an error message visible in the console or in the terminal?

comment:5 Changed 3 years ago by gk

Another thing: Are we sure the patch for https://bugzilla.mozilla.org/show_bug.cgi?id=1268726 copes with the scenario in this bug?

comment:6 Changed 3 years ago by gk

Keywords: TorBrowserTeam201610R added; TorBrowserTeam201609R removed

Moving review tickets to October.

comment:7 in reply to:  4 Changed 3 years ago by arthuredelstein

Replying to gk:

Could you check the rv of NS_NewURI()? That would fit better in the code context. Not sure what we want to do if that call failed, though. Could/should we emit an error message visible in the console or in the terminal?

That's a good suggestion. Here's a new version (same two commits rebased to the latest tor-browser) that logs a warning in the terminal:
https://github.com/arthuredelstein/tor-browser/commits/20043+2

comment:8 in reply to:  5 Changed 3 years ago by arthuredelstein

Replying to gk:

Another thing: Are we sure the patch for https://bugzilla.mozilla.org/show_bug.cgi?id=1268726 copes with the scenario in this bug?

I'm optimistic that it does. We will need to ensure that the patch for
https://bugzilla.mozilla.org/show_bug.cgi?id=1264577
includes the test we are adding here.

comment:9 Changed 3 years ago by gk

Cc: mcs brade added

Looks good to me. I am still not sure I understand why you suddenly need to add worker.js to the suffixes list, too. Could you elaborate?

mcs/brade: Could you have a look as well?

Last edited 3 years ago by gk (previous) (diff)

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

Replying to gk:

Looks good to me. I am still not sure I understand why you suddenly need to add worker.js to the suffixes list, too. Could you elaborate?

Good observation. It's because I should have included it before, but failed to do so.

comment:11 Changed 3 years ago by mcs

Kathy and I are okay with these changes. It would be clearer to us to use a comparison like aWorkerType == WorkerTypeShared instead of the less obvious aLoadGroupBehavior == OverrideLoadGroup (if it would work).

comment:12 in reply to:  11 Changed 3 years ago by arthuredelstein

Replying to mcs:

Kathy and I are okay with these changes. It would be clearer to us to use a comparison like aWorkerType == WorkerTypeShared instead of the less obvious aLoadGroupBehavior == OverrideLoadGroup (if it would work).

Good point. I added a test for WorkerTypeService for completeness, although we have WorkerTypeService disabled, and this patch will likely be excluded from the TBB/ESR52 release.

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

comment:13 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Thanks! Fixed with commits 22e6c335a74f08d37dd0e2daec9951dd4d0cc89d and cb2bd1de68d3ad252c665958616a53cb5aa35e38 on tor-browser-45.4.0esr-6.5-1.

comment:14 in reply to:  3 ; Changed 3 years ago by bugzilla

Replying to arthuredelstein:

Thanks for discovering and reporting this issue, bugzilla.

Really? I'm pleased :)

@gk too:
What about comment:2? Would it be addressed or buried in the closed tickets as many other issues?

comment:15 in reply to:  14 ; Changed 3 years ago by arthuredelstein

Replying to bugzilla:

Replying to arthuredelstein:

Thanks for discovering and reporting this issue, bugzilla.

Really? I'm pleased :)

Absolutely -- you are making valuable contributions.

@gk too:
What about comment:2?

If something in content could change a pref, that's would obviously be a very serious bug. If you can find an reproducible example, by all means open a ticket.

It sounds much more likely to me that the "dom.workers.sharedWorkers.enabled" pref had a user value set in your profile (probably by torbutton), carried over from a previous version of TBB.

Would it be addressed or buried in the closed tickets as many other issues?

If you find an issue that is lost, please open a ticket! It's much easier to deal with if there is a single issue per ticket. But please also understand we have to prioritize. And that everyone here is trying to do the right thing.

comment:16 in reply to:  15 Changed 3 years ago by cypherpunks

Replying to arthuredelstein:

It sounds much more likely to me that the "dom.workers.sharedWorkers.enabled" pref had a user value set in your profile (probably by torbutton), carried over from a previous version of TBB.

https://github.com/arthuredelstein/torbutton/commit/6c6db772020320ecd721c05025670d1c7494baab

Note: See TracTickets for help on using tickets.