Data URL Shared Workers must be shared when the parent origins match

RESOLVED FIXED in Firefox 59

Status

()

defect
P2
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: baku, Assigned: baku)

Tracking

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox57 unaffected, firefox58- unaffected, firefox59 fixed)

Details

(Whiteboard: deadline: Chrome aiming to fix in M-64)

Attachments

(2 attachments)

This is a follow up for bug 1419166.
Posted patch part 2 - WPTSplinter Review
Attachment #8934893 - Flags: review?(bkelly)
Duplicate of this bug: 1420932
Comment on attachment 8934893 [details] [diff] [review]
part 2 - WPT

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

::: testing/web-platform/tests/workers/SharedWorker_dataUrl.html
@@ +16,5 @@
> +    let count = 0;
> +    onmessage = e => {
> +      assert_equals(e.data, 1);
> +      if (++count == 2) {
> +        resolve(true);

Please add a comment about the expectations here.  Something like, "The shared workers should be separate and only see a count of 1.  We still wait for both workers to respond."

@@ +39,5 @@
> +  return new Promise(function(resolve) {
> +    let count = 0;
> +    onmessage = e => {
> +      assert_equals(e.data, ++count);
> +      if (count == 2) {

Again, please add a comment.  Maybe something like "The shared worker should be shared and therefore send an incrementing count."
Attachment #8934893 - Flags: review?(bkelly) → review+
Comment on attachment 8934892 [details] [diff] [review]
part 1 - loadingPrincipal in WorkerLoadInfo

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

Thanks for the comments and extra validation.  Looks good!

::: dom/workers/RuntimeService.cpp
@@ +2481,5 @@
>      nsCString scriptSpec;
>      nsresult rv = aLoadInfo->mResolvedScriptURI->GetSpec(scriptSpec);
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    MOZ_ASSERT(aLoadInfo->mPrincipal && aLoadInfo->mLoadingPrincipal);

MOZ_DIAGNOSTIC_ASSERT please.

::: dom/workers/ScriptLoader.cpp
@@ +1833,5 @@
>      // before doing anything else.  Normally we do this in the WorkerPrivate
>      // Constructor, but we can't do so off the main thread when creating
>      // a nested worker.  So do it here instead.
> +    mLoadInfo.mLoadingPrincipal = mWorkerPrivate->GetPrincipal();
> +    MOZ_ASSERT(mLoadInfo.mLoadingPrincipal);

MOZ_DIAGNOSTIC_ASSERT
Attachment #8934892 - Flags: review?(bkelly) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/774f1b75e350
Data URL Shared Workers must be shared when the parent origins match, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b63cb1aa01e
Data URL Shared Workers must be shared when the parent origins match - WPT, r=bkelly
https://hg.mozilla.org/mozilla-central/rev/774f1b75e350
https://hg.mozilla.org/mozilla-central/rev/6b63cb1aa01e
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Depends on: 1424336
[Tracking Requested - why for this release]:

Since bug 1419166 was uplifted to 58 we might want to uplift this as well.  Or we should decide to back bug 1419166 out from 58.

Note, if this is uplifted we also need to uplift these bugs to fix follow-on assertion issues:

bug 1424336
bug 1426162
bug 1428447
Depends on: 1457962
You need to log in before you can comment on or make changes to this bug.