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

Specify active service worker inheritance #2080

Closed
wants to merge 3 commits into from

Conversation

jungkees
Copy link
Contributor

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.

@domenic
Copy link
Member

domenic commented Nov 23, 2016

This LGTM editorially, and I will trust you on the semantics. Is someone slated to write web platform tests for this?

@domenic domenic added topic: workers needs tests Moving the issue forward requires someone to write tests labels Nov 23, 2016
@@ -77882,6 +77887,16 @@ console.assert(iframeWindow.frameElement === null);
</dl>
</li>

<li><p>Set <var>settings object</var>'s <span
data-x="concept-environment-active-service-worker">active service worker</span> to null.</p></li>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm probably missing something… when would this value be not-null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the clients that don't get to http fetch, the following steps set the value:

set settings object's active service worker to creator browsing context's active document's relevant settings object's active service worker.

and for the clients that get through http fetch, the following steps set the value:

, and settings object's active service worker to reserved environment's active service worker.

the reserved client's active service worker here is set in Handle Fetch where the registration matching is done.

source Outdated
context</span>, then set <var>settings object</var>'s <span
data-x="concept-environment-active-service-worker">active service worker</span> to <span>creator
browsing context</span>'s <span>active document</span>'s <span>relevant settings object</span>'s
<span data-x="concept-environment-active-service-worker">active service worker</span>.</p></li>
Copy link
Contributor

Choose a reason for hiding this comment

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

If I have an iframe pointing to another origin, am I right in reading it'll initially inherit my service worker via this step, but will discard it during navigation? If the target origin doesn't have a service worker, when is the active worker set to null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it'll initially inherit my service worker via this step, but will discard it during navigation?

Right.

If the target origin doesn't have a service worker, when is the active worker set to null?

the environment settings object's active service worker is copied from the reserved client's active service worker in this case. And the reserved client's active service worker should have gotten a service worker from the registration matching in Handle Fetch. So, the active service worker's value here's an initial value of the reserved client (an environment's) active service worker. Not sure if we still need "It is initially null." sentence.

@jakearchibald
Copy link
Contributor

Do we need similar steps for "set up a worker environment settings object" or do we handle inheritance for workers elsewhere?

@jungkees
Copy link
Contributor Author

jungkees commented Dec 1, 2016

I think I missed the worker case. When a script URL for a worker is a local URL, it doesn't get to http fetch. So, currently that worker client won't get a controller. (i.e. it's just null.) We have related text here - https://w3c.github.io/ServiceWorker/#selection - but I think we should add relevant steps to "set up a worker environment settings object". It should inherit either worker's document's controller or worker's worker's controller in this case I suppose.

@domenic
Copy link
Member

domenic commented Jan 13, 2017

@jungkees @jakearchibald what is the latest on this? Is anyone writing tests for it---perhaps there are tests in Blink we can import?

@jungkees
Copy link
Contributor Author

@domenic, I doubt we have tests for it. I'm working on the tests. I'll update soon.

@jakearchibald,

Do we need similar steps for "set up a worker environment settings object" or do we handle inheritance for workers elsewhere?

I've checked the algorithms for workers today. I think it doesn't need any inheritance at all. Unlike the browsing context case, a worker construction always triggers fetch. When the request doesn't get a matched SW or it doesn't reach http fetch against the local url, the worker's service worker client shouldn't have a controller. "about" scheme for worker will return a network error. "blob" and "data" schemes seem a bit controversial, but considering they don't get a controller even for top-level document clients I think it's fair not to attach controllers to them.

To clarify, for the browsing context case, only initial "about:blank" documents and srcdoc documents inherit their parent's controller. Setting an iframe's src to "about:blank" on a non-initial document won't inherit the parent's controller. That later-set "about:blank" document can even have a different origin to its parent.

@annevk
Copy link
Member

annevk commented Jan 16, 2017

That later-set "about:blank" document can even have a different origin to its parent.

Really? Do you have a test for that? Would be quite surprising.

@jungkees
Copy link
Contributor Author

jungkees commented Jan 17, 2017

Really? Do you have a test for that? Would be quite surprising.

It was my misunderstanding. The definition of document's origin says "If the Document is a non-initial "about:blank" document," The origin is determined as "the incumbent settings object when the navigate algorithm was invoked..". I thought the incumbent settings object can be different from the top-level script's environment settings object, but having checked the incumbent concept it seems the incumbent settings object is always the parent's environment setting object in this case.

Testing with the following code in Chrome says they're the same. (Firefox doesn't seem to offer document.origin yet.):

new Promise(resolve => {
  let frame = document.createElement('iframe');
  frame.src = 'blank.html';
  frame.onload = _ => { resolve(frame); };
  document.body.appendChild(frame);
}).then(frame => {
  return new Promise(resolve => {
     console.log(frame.contentWindow.document.origin);
     frame.src = 'about:blank';
     frame.onload = _ => { resolve(frame); };
  });
}).then(frame => {
  console.log(frame.contentWindow.document.origin);
});

So, should we inherit the parent's controller in this case? The patches in this PR currently set the controller to null as it goes through process a navigate fetch - meaning it creates a reserved client - and returned with no matched SW.

EDIT: testing the above code by loading a cross-origin document instead of 'blank.html' still makes the same result: document.origin after being set to 'about:blank' returns the parent's origin.

@jungkees
Copy link
Contributor Author

I found another corner case that needs clarification. With the following scenario:

  1. A document whose origin is foo.com is loaded in the parent frame.
  2. An iframe with no src set is inserted in the parent document. // Initial about:blank document's origin is inherited from the parent: foo.com.
  3. Navigate the iframe's nested browsing context to foo.com/resource.

When the navigate reaches Initialzing a new Document object step 3, the new Document reuses the Window object and the environment settings object of the initial Document rather than creating its own ones. That is, the new Document will use the active SW inherited from the parent document even if it has gotten a matched SW through the http fetch.

For this case, I think we should replace the inherited active SW with the matched SW by which the main resource has already been served. Does it makes sense? And it seems other environment's setup (e.g. id, creation URL, etc.) also needs to be overriden I guess.

Is there any particular reason the existing algorithm reuses the Window and the environment settings object only in this case in the first place?

@annevk
Copy link
Member

annevk commented Jan 17, 2017

I think that is to avoid losing global variables and getting too many history entries. @bzbarsky and maybe @smaug---- could say more about it. We really need to refactor how all these things get created and teared down, since it's a giant mess.

@smaug----
Copy link

losing global variables and losing event listeners would be bad.

@bzbarsky
Copy link
Contributor

Is there any particular reason the existing algorithm reuses the Window

Historically, because people have set variables and event listeners on the window before it's done loading, racing with the load getting its initial HTTP response headers (and the document being created). And browsers made that work regardless of whether the race was won or lost.

It's very much worth investigating what actual browser behavior around this stuff is. The spec was written more or less to match Gecko, but I know for a fact Chrome/WebKit have different (much more confusing, which is why it was not specced) behavior, and Edge/IE have yet a third behavior in the window.open case; dunno about the iframe case.

It's possible that there are web-compatible simplifications to the behavior here that would make all this a non-issue, though unlikely: Hixie is pretty good at testing various edge-cases...

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.
 - Move the initial inheritance into the non navigate fetch case
 - Add steps to handle about:blank Document cases
  . When navigating an iframe with the initial about:blank Document, use
    the matched SW if it has one.
  . When navigating an iframe to a non-initial about:blank Document,
    inherit the parent's SW if it has a parent.
jungkees added a commit to jungkees/web-platform-tests that referenced this pull request Jan 24, 2017
Loading an iframe without involving a navigate fetch inherits its
parent's active service worker, if it has one. By this rule:
 - Initial about:blank Document on iframe inherits its parent's SW
 - iframe srcdoc Document inherits its parent's SW

However, the navigate fetch through http fetch still wins. So, if a
client obtains a matched SW during the navigation, the matched SW
replaces the inherited SW.

An exception:
 - Navigating an iframe to a non-initial about:blank Document inherits
   the parent's SW

Related work: whatwg/html#2080.
@jungkees
Copy link
Contributor Author

@smaug----, @bzbarsky, thanks for the answers.

It's very much worth investigating what actual browser behavior around this stuff is.

It surely is an area that needs investigation.

@domenic, @jakearchibald, @annevk,

I pushed a new commit to cover two corner cases that were mentioned in the above comments:

  • For navigating an iframe with the initial about:blank Document, I added steps to replace the inherited active SW with the matched SW (which the main resource has been served by.)
  • For navigating an iframe to a non-initial about:blank Document, I added steps to make it inherit the parent's SW rather than leaving it to null set during the navigate fetch.

And added the wpt tests: web-platform-tests/wpt#4610.

@domenic
Copy link
Member

domenic commented Feb 7, 2017

Ping reviewers @jakearchibald and @annevk; I don't feel qualified to review this beyond just editorial matters. I see @mkruisselbrink is reviewing the tests; maybe he can help as well.

jungkees added a commit to jungkees/web-platform-tests that referenced this pull request Mar 24, 2017
Loading an iframe without involving a navigate fetch inherits its
parent's active service worker, if it has one. By this rule:
 - Initial about:blank Document on iframe inherits its parent's SW
 - iframe srcdoc Document inherits its parent's SW

However, the navigate fetch through http fetch still wins. So, if a
client obtains a matched SW during the navigation, the matched SW
replaces the inherited SW.

An exception:
 - Navigating an iframe to a non-initial about:blank Document inherits
   the parent's SW

Related work: whatwg/html#2080.
@jungkees
Copy link
Contributor Author

jungkees commented Jul 4, 2017

I think it's difficult to rebase, review, and discuss on this PR after a long pause of work. I created a new PR for this effort. Please come to #2809.

@jungkees jungkees closed this Jul 4, 2017
@jungkees jungkees deleted the embeddedclient-fix branch July 5, 2017 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests Moving the issue forward requires someone to write tests topic: workers
Development

Successfully merging this pull request may close these issues.

6 participants