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

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Ehsan, Assigned: baku)

Tracking

({dev-doc-complete})

unspecified
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

4 years ago
No description provided.
Reporter

Updated

4 years ago
See Also: → 1173467
Assignee

Comment 1

4 years ago
Posted 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-
Assignee

Comment 4

4 years ago
Posted 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+
Assignee

Comment 6

4 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.
https://hg.mozilla.org/mozilla-central/rev/fafc26e98274
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Reporter

Updated

4 years ago
Depends on: 1179548
You need to log in before you can comment on or make changes to this bug.