Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Specify active service worker inheritance #2809

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

@jungkees
Copy link
Contributor

@jungkees jungkees commented Jul 4, 2017

This adds steps to make clients that have creator browsing contexts to
inherit the active service worker from their creators. While they have
an initial inherited active service worker, if one exists, navigation
matching still always wins. So, if the navigation goes through HTTP
fetch, the inherited active service worker is replaced by the matched
result. Also, if an iframe has a sandbox attribute set without
allow-same-origin token, the inherited active service worker is set to
null.

Related issue: w3c/ServiceWorker#765.


馃挜 Error: Wattsi server error 馃挜

PR Preview failed to build. (Last tried on Jan 15, 2021, 7:57 AM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

馃毃 Wattsi Server - Wattsi Server is the web service used to build the WHATWG HTML spec.

馃敆 Related URL

Parsing MDN data...
Parsing...



If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

This adds steps to make clients that have creator browsing contexts to
inherit the active service worker from their creators. While they have
an initial inherited active service worker, if one exists, navigation
matching still always wins. So, if the navigation goes through HTTP
fetch, the inherited active service worker is replaced by the matched
result. Also, if an iframe has a sandbox attribute set without
allow-same-origin token, the inherited active service worker is set to
null.

Related issue: w3c/ServiceWorker#765.
@jungkees
Copy link
Contributor Author

@jungkees jungkees commented Jul 4, 2017

  • I plan to revisit web-platform-tests/wpt#4610 for testing.
  • I'll address necessary changes for the environment's execution ready flag in a separate PR when we have more clear picture here.
  • This change includes:
    • Inherit creator active service worker when creating a new browsing context.
    • Transfer the creation URL and active service worker of the reserved environment gotten through the http fetch to the (reused) initial about:blank client.
    • Nullify the client's active service worker when the sandboxed origin browsing context flag is set.

Note: I just came up with an idea (though I'm not sure it's better or feasible). For the initial about:blank client navigation, can we set the request's reserved client to the initial client (the environment settings object created during the creation of the browsing context) directly instead of creating and setting a new environment? (See https://html.spec.whatwg.org/#process-a-navigate-fetch step 4 and 5.) I'll think about that option too.

@jungkees
Copy link
Contributor Author

@jungkees jungkees commented Jul 4, 2017

It seems I can't set the reviewers. (Has the policy been changed?)

@annevk, @jakearchibald, @domenic, I'd appreciate your review.

/cc @wanderview @mattto

@annevk
Copy link
Member

@annevk annevk commented Jul 5, 2017

No, the reviewers feature is broken likely due to the size of the source resource. We've raised it with GitHub, but nothing thus far.

Copy link
Member

@annevk annevk left a comment

In general I'm a little hesitant to move forward with this, since all these algorithms need refactoring first. Once we make it clearer how documents and global objects are allocated it would be much easier to plug in service workers.

I wonder if it would not be better to wait with this PR until that is done. I am planning to actively work on that, for what it's worth.

<ol>
<li>
<p>If the new <code>Document</code> is not the <code>about:blank</code>
<code>Document</code>, then:</p>
Copy link
Member

@annevk annevk Jul 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this conditional.

Copy link
Contributor Author

@jungkees jungkees Jul 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to exclude the case where the navigate was done against the (non-initial) about:blank Document. (This is basically the part that overrides the active service worker and the creation URL of the "old" environment settings object with the new values gotten from the navigate fetch.)

Thinking on your comment, I noticed that the about:blank document should have a unique origin. So, I think this condition isn't needed.

Copy link
Contributor Author

@jungkees jungkees Jul 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed this in the follow-up commit.


<ol>
<li><p>Let <var>settingsObject</var> be <var>browsingContext</var>'s <span>active
document</span>'s <span>environment settings object</span>.</p></li>
Copy link
Member

@annevk annevk Jul 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is the "old" document?

Copy link
Member

@annevk annevk Jul 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though I guess for the settings object that doesn't matter...

Copy link
Contributor Author

@jungkees jungkees Jul 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. This should be the old one because the new one isn't installed yet. This is just to get the handle to the settings object anyway as you pointed.

<li><p>Set <var>settingsObject</var>'s <span
data-x="concept-environment-creation-url">creation URL</span> to
<var>reservedEnvironment</var>'s <span data-x="concept-environment-creation-url">creation
URL</span>.</p></li>
Copy link
Member

@annevk annevk Jul 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be done either way? Is this something that's wrong today?

Copy link
Contributor Author

@jungkees jungkees Jul 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We reuse the "old" environment settings object for the new Document in this case, but don't have any steps to change the creation URL and the active service worker. Currently without adding this, the resulting (old) environment settings object won't get a controller even though the navigate fetch matched and delivered one.

@jungkees
Copy link
Contributor Author

@jungkees jungkees commented Jul 5, 2017

I wonder if it would not be better to wait with this PR until that is done. I am planning to actively work on that, for what it's worth.

@annevk, I think I'm pretty clear about the behavior I'm trying to add in this PR: 1) a child/auxiliary client inherits its parent's SW 2) in the initial client reuse case, we should set the initial client's SW and the creation URL to the values we got from the navigate fetch 3) a sandboxed iframe without allow-same-origin won't get a controller.

But I guess it'll still take time to land this PR including the effort for testing. So, I'm fine with your plan. Just keep me posted in the related PRs.

@domenic
Copy link
Member

@domenic domenic commented Jul 5, 2017

FWIW I'd rather land it earlier than wait; I think we need to not hold up ongoing integration work on straightening out our story here.

I don't feel super-capable of reviewing this, so unless there's something specific I could help with I'll defer to @annevk...

source Outdated
@@ -82208,7 +82201,7 @@ State: &lt;OUTPUT NAME=I>1&lt;/OUTPUT> &lt;INPUT VALUE="Increment" TYPE=BUTTON O
</li>

<li><p><span>Set up a window environment settings object</span> with <var>realm execution
context</var> and <var>reservedEnvironment</var>, if present.</p></li>
Copy link
Contributor Author

@jungkees jungkees Jul 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed "if present" as reservedEnvironment should always be present in this step.

@annevk
Copy link
Member

@annevk annevk commented Jul 6, 2017

@domenic I appreciate the drive to just keep moving, but I think we'll end up with another #2687. As long as we haven't properly defined the lifetime of documents and in particular when they are created and how, we're not helping anyone by adding more text to the existing algorithms that have lots of known issues.

@jungkees
Copy link
Contributor Author

@jungkees jungkees commented Jul 3, 2018

We need a spec for the service worker inheritance. I think we can still work on this PR.

I wrote a glitch to test the current behavior: https://sw-inheritance.glitch.me/.

  • For the initial about:blank iframe
    • Edge and Chrome return null.
    • Firefox inherits the parent's controller.
  • For the sandboxed iframe
    • All throw when accessing navigator.serviceWorker.controller.
  • For the sandboxed iframe with allow-same-origin
    • All return null.

The expected behavior the PR suggests is inherit / null (and throw as-is) / inherit, respectively.

/cc SW folks: @jakearchibald @wanderview @aliams @slightlyoff @mattto @cdumez (sorry I haven't tested with Safari. Please add.)

@wanderview
Copy link
Member

@wanderview wanderview commented Jul 3, 2018

The expected behavior the PR suggests is inherit / null (and throw as-is) / inherit, respectively.

I think thats correct.

The firefox behavior for the sandboxed iframe with allow-same-origin can probably be fixed by changing this line:

https://searchfox.org/mozilla-central/rev/97d488a17a848ce3bebbfc83dc916cf20b88451c/docshell/base/nsDocShell.cpp#13815

To check (mSandboxFlags & SANDBOXED_ORIGIN) instead of any mSandboxFlags value.

cc @asutherland @mrbkap

@wanderview
Copy link
Member

@wanderview wanderview commented Jul 3, 2018

The expected behavior the PR suggests is inherit / null (and throw as-is) / inherit, respectively.

Safari TP 59 matches the expectations.

Note, though, the sandbox error is more about accessing the cross-origin iframe's contentWindow. It might be better to change the test so that the child frame postMessage()'s the controller scriptURL to the parent.

@jungkees
Copy link
Contributor Author

@jungkees jungkees commented Jul 3, 2018

Note, though, the sandbox error is more about accessing the cross-origin iframe's contentWindow. It might be better to change the test so that the child frame postMessage()'s the controller scriptURL to the parent.

That's right. But we shouldn't make the iframe navigate. Maybe should I use srcdoc that postMessage() to the parent? In that case, the inherited controller should hold, right?

@asutherland
Copy link

@asutherland asutherland commented Jul 3, 2018

Firefox bug 1279406 tracks the sandboxed iframe with "allow-same-origin" inheritance and I've updated it to link to @wanderview's comment and suggested fix. Thanks, Ben!

@wanderview
Copy link
Member

@wanderview wanderview commented Jul 3, 2018

But we shouldn't make the iframe navigate.

Oh yeah, it would be hard to make a sandboxed about:blank post message anything.

Another way to observe it would be to use clients.matchAll() from the SW and look at the list of controlled clients.

@jungkees
Copy link
Contributor Author

@jungkees jungkees commented Jul 3, 2018

Another way to observe it would be to use clients.matchAll() from the SW and look at the list of controlled clients.

Right. postMessage() from sandboxed iframe won't work. So, this would be a way to go.

Base automatically changed from master to main Jan 15, 2021
@annevk
Copy link
Member

@annevk annevk commented Apr 28, 2021

@antosart this is an interesting case for policy containers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants