-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Specify active service worker inheritance #2809
base: main
Are you sure you want to change the base?
Conversation
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.
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. |
It seems I can't set the reviewers. (Has the policy been changed?) @annevk, @jakearchibald, @domenic, I'd appreciate your review. /cc @wanderview @mattto |
No, the reviewers feature is broken likely due to the size of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@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. |
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: <OUTPUT NAME=I>1</OUTPUT> <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> |
There was a problem hiding this comment.
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.
@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. |
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/.
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.) |
I think thats correct. The firefox behavior for the sandboxed iframe with allow-same-origin can probably be fixed by changing this line: To check |
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. |
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? |
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! |
Oh yeah, it would be hard to make a sandboxed about:blank post message anything. Another way to observe it would be to use |
Right. postMessage() from sandboxed iframe won't work. So, this would be a way to go. |
@antosart this is an interesting case for policy containers. |
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
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.