Closed Bug 1177621 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: DOM: Workers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: ehsan, Assigned: baku)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

No description provided.
See Also: → 1173467
Attached patch pb.patch (obsolete) — Splinter Review
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-
Attached patch pb.patchSplinter Review
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+
> 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.
https://hg.mozilla.org/mozilla-central/rev/fafc26e98274
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1179548
You need to log in before you can comment on or make changes to this bug.