Do not share a SharedWorker between a private and a non-private document

RESOLVED FIXED in Firefox 41

Status

()

Core
DOM: Workers
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: baku)

Tracking

({dev-doc-complete})

unspecified
mozilla41
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
See Also: → bug 1173467
(Assignee)

Comment 1

2 years ago
Created attachment 8626536 [details] [diff] [review]
pb.patch
Attachment #8626536 - Flags: review?(nsm.nikhil)
Can you base this on top of my patches in bug 1173467?  I already built the WorkerPrivate private browsing flag stuff there.
Comment on attachment 8626536 [details] [diff] [review]
pb.patch

Review of attachment 8626536 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/WorkerPrivate.cpp
@@ +2335,5 @@
>    , mPrincipalIsSystem(false)
>    , mIsInPrivilegedApp(false)
>    , mIsInCertifiedApp(false)
>    , mIndexedDBAllowed(false)
> +  , mPrivateBrowsing(true)

Why default mPrivateBrowsing to true?

@@ +5085,5 @@
>        loadInfo.mIsInCertifiedApp = (appStatus == nsIPrincipal::APP_STATUS_CERTIFIED);
>        loadInfo.mFromWindow = true;
>        loadInfo.mWindowID = globalWindow->WindowID();
>        loadInfo.mIndexedDBAllowed = IDBFactory::AllowedForWindow(globalWindow);
> +      loadInfo.mPrivateBrowsing = nsContentUtils::IsInPrivateBrowsing(document);

So I pull the private browsing flag from the mLoadGroup set in SetPrincipal().  I think this might be more consistent across all cases.  Its also important to be in sync with the LoadGroup since other things are going to inspect it for behavior.
Attachment #8626536 - Flags: feedback-
(Assignee)

Comment 4

2 years ago
Created attachment 8626614 [details] [diff] [review]
pb.patch

The reason why by default I set mPrivateBrowsing to true is to prevent the exposing of ServiceWorkers in case WorkerLoadInfo is not fully initialized.
Attachment #8626536 - Attachment is obsolete: true
Attachment #8626536 - Flags: review?(nsm.nikhil)
Attachment #8626614 - Flags: review?(nsm.nikhil)
Comment on attachment 8626614 [details] [diff] [review]
pb.patch

Review of attachment 8626614 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/ServiceWorkerManager.cpp
@@ +3735,5 @@
>    info.mPrincipal = aPrincipal;
>  
>    info.mIndexedDBAllowed =
>      indexedDB::IDBFactory::AllowedForPrincipal(aPrincipal);
> +   info.mPrivateBrowsing = false;

Why is this required when the load group on the info can be used to infer it?
Attachment #8626614 - Flags: review?(nsm.nikhil) → review+
(Assignee)

Comment 6

2 years ago
> Why is this required when the load group on the info can be used to infer it?

Because there is an assertion in GenerateSharedWorkerKey() and the WorkerLoadInfo will be updated only when the script is fully loaded (from the network or from the cache) but that is too late and the key in mSharedWorkerInfos has already been created.
(Assignee)

Comment 7

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fafc26e98274
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/fafc26e98274
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1179548
Added a note about this to the appropriate places:

https://developer.mozilla.org/en-US/docs/Web/API/SharedWorker
https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Using_web_workers#Shared_workers
https://developer.mozilla.org/en-US/Firefox/Releases/41#Miscellaneous

Does the wording sound ok? Cheers!
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.