Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Firefox 41



DOM: Workers
2 years ago
2 years ago


(Reporter: Ehsan, Assigned: baku)




Firefox Tracking Flags

(firefox41 fixed)



(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
See Also: → bug 1173467

Comment 1

2 years ago
Created attachment 8626536 [details] [diff] [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]

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-

Comment 4

2 years ago
Created attachment 8626614 [details] [diff] [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]

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+

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.

Comment 7

2 years ago
Keywords: dev-doc-needed
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:

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.