Isolate mediasource and blob URLs in all contexts
In #15502 (moved) we isolated blob URLs and disabled them in Worker contexts. This ticket is a follow-up to get the URL-bar-domain-based isolation implemented in Worker environments.
Activity
Please see the follow branch for review:
https://github.com/arthuredelstein/tor-browser/commits/16429+7
It contains several related patches:
- A fixup patch that gets the blob URI regression tests working after they broke in the FF31->FF38 transition (fixes #16416 (moved)).
- A "revert" patch that removes the old blob URI isolation patch (that disabled blob URIs in web workers).
- A new version of the blob URI patch that isolates blob URIs in workers without disabling them (#16429 (moved)). The new patch also fixes mediasource (and mediastream) URIs (#15703 (moved)).
- A second fixup patch for the blob URI regression tests that makes sure they test blob URI isolation in Web workers (#16429 (moved)).
- A new patch that tests mediasource URI isolation (#15703 (moved)).
I just confirmed that, if I revert the (new) #15703 (moved) patch, that the regression tests for both blob and mediasource URIs fail. (This is the desired behavior.)
Some issues:
- In URL::RevokeObjectURL(), you can have a null pointer deref if window is null, since you try to obtain the document before checking window.
- We probably should have some error checking in your GetFirstPartyHost() functions, if nothing else to differentiate between the case where isolation is disabled (NS_OK, but empty isolation URI) and failure (NS_FAILED(rv), with empty isolation URI). The returned host should probably also represent these two cases as different special string keys?
- I found a meta-bug (https://bugzilla.mozilla.org/show_bug.cgi?id=60697) where Mozilla was trying to remove static global uses of nsCOMPtr. I'm wondering if this is limited to static initialization or if we should be using something other than nsCOMPtr for gThirdPartyUtilService. The bug has been dormant for years, though...
Replying to mikeperry:
Some issues:
- In URL::RevokeObjectURL(), you can have a null pointer deref if window is null, since you try to obtain the document before checking window.
- We probably should have some error checking in your GetFirstPartyHost() functions, if nothing else to differentiate between the case where isolation is disabled (NS_OK, but empty isolation URI) and failure (NS_FAILED(rv), with empty isolation URI). The returned host should probably also represent these two cases as different special string keys?
- I found a meta-bug (https://bugzilla.mozilla.org/show_bug.cgi?id=60697) where Mozilla was trying to remove static global uses of nsCOMPtr. I'm wondering if this is limited to static initialization or if we should be using something other than nsCOMPtr for gThirdPartyUtilService. The bug has been dormant for years, though...
These are all good points. I've attempted to fix them. The new version is here: https://github.com/arthuredelstein/tor-browser/commits/16429+8
Replying to mikeperry:
The new branch 16429+8 looks OK to me. I would like a second opinion from mcs+brade before merge, though.
A patch I proposed for Mozilla Bug 1078657 was recently accepted in mozilla-central. This patch adds a file, SpanwTask.js, that makes it simple to use coroutines in mochitests. So here's a new version of the 16429 branch, with the patch backported and the regression tests refactored to make use of SpawnTask.js. Otherwise everything in this branch is the same.
https://github.com/arthuredelstein/tor-browser/commits/16429+9
-
Generalizing the ticket title so we can dup #15703 (moved) here.
Trac:
Summary: Isolate blob URLs in Worker contexts to Isolate mediasource and blob URLs in all contexts 
Kathy and I reviewed all of these changes and they look OK to us. Just a few comments:
- Are there cases where the isolation key will be "--ISOLATION-FAILED-" when it would be better to just fail? For example, we could fail the creation of a blob URL if we cannot get an isolation key.
- A nit: replace "isolationKey" with "aIsolationKey" in the nsHostObjectProtocolHandler::AddDataEntry() parameter list.
- The messageSent variable is not really used inside dom/base/test/bug15703_page_retrieve.html; you could just remove it.
Replying to mcs:
Kathy and I reviewed all of these changes and they look OK to us. Just a few comments:
- Are there cases where the isolation key will be "--ISOLATION-FAILED-" when it would be better to just fail? For example, we could fail the creation of a blob URL if we cannot get an isolation key.
- A nit: replace "isolationKey" with "aIsolationKey" in the nsHostObjectProtocolHandler::AddDataEntry() parameter list.
- The messageSent variable is not really used inside dom/base/test/bug15703_page_retrieve.html; you could just remove it.
Thanks for the review and these helpful suggestions. I've made changes to address each of them. Here's the new branch:
https://github.com/arthuredelstein/tor-browser/commits/16429+11
-
Ok, I took a look at these and they seem good, thanks! Merged for 5.0a4.
Out of curiosity, I also noticed you added preliminary isolation for fetch? Was that to test #16326 (moved), or do you believe you've solved the odd caching problems there?
Trac:
Status: needs_review to closed
Resolution: N/A to fixed Replying to mikeperry:
Ok, I took a look at these and they seem good, thanks! Merged for 5.0a4.
Out of curiosity, I also noticed you added preliminary isolation for fetch? Was that to test #16326 (moved), or do you believe you've solved the odd caching problems there?
This was intended to protect against using Fetch with a blob URI. But I think we then realized that Fetch is not active until FF39. So it will hopefully serve as a reminder when we rebase to FF45-ESR.
mentioned in issue #16577 (moved)
mentioned in issue #16781 (moved)