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

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Comment 1

[Andrea Marchesini [:baku]]

Attachment: pb.patch

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Can you base this on top of my patches in bug 1173467?  I already built the WorkerPrivate private browsing flag stuff there.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: 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.

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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?

Comment 6

[Andrea Marchesini [:baku]]

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

Comment 7

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fafc26e98274

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fafc26e98274

Comment 9

[Chris Mills [:cmills]]

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!