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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discard the environment after a failed navigation or worker start. #3723

Merged
merged 8 commits into from Jun 6, 2018

Conversation

mfalken
Copy link
Contributor

@mfalken mfalken commented May 30, 2018

This will allow the service worker spec to "wait for the environment to be discarded" when the navigation or worker start fails, needed for w3c/ServiceWorker#1315

cc @domenic


/browsing-the-web.html ( diff )
/webappapis.html ( diff )
/workers.html ( diff )

This will allow specifications to do things like "wait for
the environment to be destroyed" as needed in
w3c/ServiceWorker#1315.
source Outdated
@@ -86333,6 +86334,10 @@ interface <dfn>ApplicationCache</dfn> : <span>EventTarget</span> {
unset.</p></dd>
</dl>

<p>To <dfn data-x="discard an environment">discard an environment</dfn>, there are no explicit
Copy link
Member

Choose a reason for hiding this comment

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

I think it makes more sense to define the "environment discarding steps". Cf https://dom.spec.whatwg.org/#concept-node-adopt-ext, https://html.spec.whatwg.org/#unloading-document-cleanup-steps.

Also nits on the <dfn> syntax: data-x="" is not needed if the text inside the element is already good. And, you should add data-export="" so that it automatically becomes usable in Bikeshed-based specs (like service worker).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not too sure I understand the suggestion. The steps will be an empty list of steps. I just want the service worker spec to say "wait until either the environment discarding steps run, or for the execution ready flag to be set".

Also thanks for the syntax tips... is there documentation somewhere about data-x and data-export etc? I couldn't find it from quick searching.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How is something like this?

<p>The <dfn data-export="">environment discarding steps</dfn> is an empty list of steps. It is 
defined centrally here so specifications can wait for the point of time when they are run.</p>

Copy link
Member

Choose a reason for hiding this comment

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

Did you see the DOM example I linked? You're not defining an actual set of steps, empty or not. You're defining the concept of "environment discarding steps", which other specs can reference.

This is editorial and I can tidy it up later if you want. But basically following the verbiage in DOM would work well, I was thinking.

source Outdated
completion value.</p>
at <var>worker</var>, <span data-x="discard an environment">discard</span> <var>inside
settings</var>, and return. Otherwise, continue the rest of these steps after the algorithm's
asynchronous completion, with <var>script</var> being the asynchronous completion value.</p>
Copy link
Member

@domenic domenic May 30, 2018

Choose a reason for hiding this comment

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

This is kind of a preexisting problem, but now it's extra confusing; it looks like it's saying "queue a task to (fire an event, discard, and return)", whereas the intended semantics are "(queue a task to fire an event), (discard), and (return)".

I'd suggest introducing a nested list:

<p>If ..., then</p>

<ol>
 <li><p>Queue...</p></li>
 <li><p>Discard...</p></li>
 <li><p>Return.</p></li>
</ol>

<p>Otherwise, ...</p>

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

A few editorial issues. Maybe @annevk can help review that the concept is inserted in the right places?

source Outdated
@@ -86333,6 +86334,10 @@ interface <dfn>ApplicationCache</dfn> : <span>EventTarget</span> {
unset.</p></dd>
</dl>

<p>To <dfn data-x="discard an environment">discard an environment</dfn>, there are no explicit
steps to take. It is defined centrally here so specifications can wait for an environment to be
discarded.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Please add a note here that this is fairly limited as it doesn't run for all environments, just a select few.

@annevk annevk mentioned this pull request May 30, 2018
@annevk
Copy link
Member

annevk commented May 30, 2018

@wanderview should also review this, judging from the service worker PR.

@mfalken
Copy link
Contributor Author

mfalken commented Jun 1, 2018

Thanks for the comments. Please take another look.

Copy link
Member

@wanderview wanderview left a comment

Choose a reason for hiding this comment

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

I kind of feel like this should also be incorporated into window unloading and worker terminating algorithms for clients that do reach execution ready. For example, the normal unloading/terminating steps happen and then they trigger the discarding. I guess this could be tackled later.

Assuming we don't do what I describe above, I think Handle Service Worker Client Unload should also reference this discarding step:

https://w3c.github.io/ServiceWorker/#on-client-unload-algorithm

Since step 12.6 of Handle Fetch starts controlling the reserved client:

https://w3c.github.io/ServiceWorker/#on-fetch-request-algorithm

We need to stop controlling the reserved client if its discarded.

Also, depending on how things end up in w3c/ServiceWorker#1316, a cross-origin redirect should also trigger the discard logic.

source Outdated
@@ -86333,6 +86335,14 @@ interface <dfn>ApplicationCache</dfn> : <span>EventTarget</span> {
unset.</p></dd>
</dl>

<p>The <dfn data-export="">environment discarding steps</dfn> for an environment is an empty list
of steps. It is defined centrally here so specifications can wait for the point of time when they
Copy link
Member

Choose a reason for hiding this comment

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

nit: s/point of time/point in time/

@mfalken
Copy link
Contributor Author

mfalken commented Jun 5, 2018

@domenic Thanks, how is this now?

@wanderview Thanks! I agree with what you suggest and think it can be tackled later.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

This looks lovely, thank you!

@domenic
Copy link
Member

domenic commented Jun 5, 2018

Should I hold off on merging until you've updated w3c/ServiceWorker#1315 and confirmed it works with this phrasing, or should I go ahead and merge now?

@mfalken
Copy link
Contributor Author

mfalken commented Jun 5, 2018

Thanks! It's OK to merge now. I'm pretty sure it'll worth with service worker.

@domenic domenic merged commit 9592eb5 into whatwg:master Jun 6, 2018
@mfalken mfalken deleted the discard branch June 7, 2018 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants