SharedWorker uses catchall circuit

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

added TorBrowserTeam201610R component::applications/tor browser owner::tbb-team priority::high resolution::fixed severity::major status::closed tbb-linkability type::defect labels

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.

Trac:
Keywords: tbb-pref deleted, N/A added
Cc: N/A to arthuredelstein
Priority: Medium to High
Severity: Normal to Major

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, so some website added it).

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.

Trac:
Status: new to needs_review
Keywords: N/A deleted, TorBrowserTeam201609R added

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?

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

Moving review tickets to October.

Trac:
Keywords: TorBrowserTeam201609R deleted, TorBrowserTeam201610R added

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

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.

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?

Trac:
Cc: arthuredelstein to arthuredelstein, mcs, brade

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.

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

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

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

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

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?

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.

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

closed

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