the tor browser does isolation of shared workers. this bug is to implement the same isolation, starting with the Tor Browser patch and refactoring it to use origin attributes.

Here's a link that tracks Tor Browser's latest version of the patch: https://torpat.ch/15564

Dave, since you already uploaded a patch for that I am assigning this bug to.

Per baku, Shared Workers already take OriginAttributes into account.

(In reply to Tanvi Vyas [:tanvi] from comment #4) > Per baku, Shared Workers already take OriginAttributes into account. I have tested shared workers in the Bug 1264593, it shows that shared workers are not segregated by the OriginAttributes. I think the reason is that the key of the shared worker does not reference originAttributes [1]. This could be fixed by adding originAttributes as a part of the key. [1] http://searchfox.org/mozilla-central/source/dom/workers/RuntimeService.cpp#241

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99b08dfc361e

WIP

changing to HG patch format.

(In reply to Dave Huseby [:huseby] from comment #9) > Created attachment 8789102 [details] [diff] [review] > WIP Bug_1268726.patch > > changing to HG patch format. Is this patch on top of the patches that landed in Bug 1294336 by Tim? Was the work in that bug not sufficient to isolate shared workers by all Origin Attributes?

Hi Tanvi, I took over the bug that Tim had and closed it as a duplicate. His patch, though r+, was not landed and was dropped in favor of my implementation for various reasons. After fighting with mercurial and review board, I'm back to git and raw patches. That's why this seems like it is a patch that is showing up late to the show. Sorry for the confusion. --dave

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99b08dfc361e

Comment on attachment 8789102 [details] [diff] [review] Isolates shared workers by origin attributes. Review of attachment 8789102 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/WorkerPrivate.cpp @@ +4122,5 @@ > loadInfo.mDomain = aParent->Domain(); > loadInfo.mFromWindow = aParent->IsFromWindow(); > loadInfo.mWindowID = aParent->WindowID(); > loadInfo.mStorageAllowed = aParent->IsStorageAllowed(); > + loadInfo.mPrivateBrowsing = aParent->IsInPrivateBrowsing(); // looks at origin attriubtes 1. attributes 2. can we remove this completely? Probably yes. Do you mind to file a follow up? ::: dom/workers/WorkerPrivate.h @@ +787,5 @@ > + } > + > + const PrincipalOriginAttributes& > + OriginAttributesRef() const > + { MOZ_ASSERT(NS_IsMainThread());

Filed a follow up bug to remove the IsInPrivateBrowsing: Bug 1302566 Fixed the nit to add MOZ_ASSERT(NS_IsMainThread()); Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=890e5405e361

r+ :baku already. fixed the nit.

That new assert you added is firing like crazy on the last Try push. Please verify that the run is green before requesting checkin.

MOZ_ASSERT(NS_IsMainThread()); This assert? Then we have a problem! mPrincipal can not be touched outside the main-thread. I guess you need to cache the full OriginAttributes in the loadInfo.

Adding OA tag since this is for Origin Attributes in general, and not just first party isolation.

Hrm...will modify the patch and do a new try ASAP.

This patch includes my first pass at removing IsInPrivateBrowsing functions from the shared worker state and adds access to origin attributes of a doc/channel via UserContentUtils. This also adds caching of origin attributes in the working load info and the worker private structs to avoid accessing the principal so the code works in both parent and child processes. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f31eace6141b

Comment on attachment 8792011 [details] [diff] [review] patch that includes removing IsInPrivateBrowsing functions Baku, I've decided to stop here in this bug so that this can get landed before the fork. I left the IsInPrivateBrowsing in nsContentUtils because there are other subsystems that rely on it. I will remove that and refactor the other dependent systems in Bug 1302566

Comment on attachment 8792011 [details] [diff] [review] patch that includes removing IsInPrivateBrowsing functions Review of attachment 8792011 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsContentUtils.h @@ +788,5 @@ > /** > + * Returns origin attributes of the document. > + **/ > + static mozilla::PrincipalOriginAttributes > + OriginAttributesRef(nsIDocument* aDoc); This is not a reference. Call it GetOriginAttributes or OriginAttributes. @@ +794,5 @@ > + /** > + * Returns origin attributes of the load group. > + **/ > + static mozilla::PrincipalOriginAttributes > + OriginAttributesRef(nsILoadGroup* aLoadGroup); same here. ::: dom/workers/ScriptLoader.cpp @@ +410,5 @@ > nsCOMPtr<nsIGlobalObject> mSandboxGlobalObject; > nsTArray<RefPtr<CacheScriptLoader>> mLoaders; > > nsString mCacheName; > + PrincipalOriginAttributes mAttrs; mOriginAttributes; @@ +1478,5 @@ > mCacheStorage = > CacheStorage::CreateOnMainThread(mozilla::dom::cache::CHROME_ONLY_NAMESPACE, > mSandboxGlobalObject, > + aPrincipal, > + false, /* can't be true, won't get here. */ /* privateBrowsing cannot be true here */ or... I mean, write 'privateBrowsing' ::: dom/workers/ServiceWorkerInfo.h @@ +30,5 @@ > const nsCString mScope; > const nsCString mScriptSpec; > const nsString mCacheName; > ServiceWorkerState mState; > + PrincipalOriginAttributes mAttrs; also here. mOriginAttributes; @@ +105,5 @@ > return mState; > } > > + const PrincipalOriginAttributes& > + OriginAttributesRef() const OriginAttributes() const ::: dom/workers/WorkerPrivate.cpp @@ +1742,2 @@ > , mServiceWorkersTestingInWindow(false) > + , mAttrs() This should be needed, right? ::: dom/workers/WorkerPrivate.h @@ +783,5 @@ > return mLoadInfo.mStorageAllowed; > } > > + const PrincipalOriginAttributes& > + OriginAttributesRef() const Nit: up to you, but maybe OriginAttributes() ? @@ +788,2 @@ > { > + return mLoadInfo.mAttrs; mOriginAttributes ::: dom/workers/Workers.h @@ +272,2 @@ > bool mServiceWorkersTestingInWindow; > + PrincipalOriginAttributes mAttrs; call it mOriginAttributes

r+ baku, updated patch with review fixes. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=61a6d87500be

The try looks good. Flagging for check in.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/39b278f56017 isolate shared worker by first party domain. r=baku