Eliminate usage of entry and incumbent settings objects

[domenic]

See https://readable-email.org/list/public-script-coord/topic/multiple-globals-and-you. We'd like to stop using these in new specs. In addition, it seems to be the case that most places that use them, are not actually implemented that way in browsers.

It would be good to do an audit of this spec and see which globals are actually implemented in browsers, then update the spec to use those. In the rare cases where incumbent and entry are actually implemented as specced, it would be ideal to change implementations if possible.

For an example of how you can test implementations, see e.g. whatwg/html#1472 and whatwg/html#1473

[jungkees]

I'd need to check them thoroughly but presume many of them actually implemented entry and incumbent. I changed navigator.serviceWorker.register(url) and navigator.serviceWorker.getRegistration(url) to use entry for parsing url at least.

For that change, I was referencing the example code in https://www.w3.org/TR/2016/WD-html51-20160503/webappapis.html#calling-scripts section the corresponding part of which in WHATWG version is now replaced with your recent patches that clarify those concepts.

Having tried the example code with location.assign(), it turned out Chrome and IE (didn't try with Edge yet) use entry for parsing the url and Firefox uses current. (I saw you filed the Location object behavior in whatwg/html#1431)

To sort them out in SW spec, I'd want to check once again if the goal is to make them all use current instead of entry and incumbent for both parsing urls and security checks? And don't we need any implementer/developer feedback on the behavior change? (I agree we should sort them out sooner than later but somewhat worry about the impact.)

[domenic]

To sort them out in SW spec, I'd want to check once again if the goal is to make them all use current instead of entry and incumbent for both parsing urls and security checks?

Sometimes it may be more appropriate to use relevant. Boris has argued that for URL parsing in particular that is better. (Note that inside constructors, the relevant settings object of the object being created is the current settings object, so we just use the latter.)

And don't we need any implementer/developer feedback on the behavior change?

So far in this multi-globals work, it seems to not impact developers at all, since they mostly don't do weird multi-global things on purpose. Implementers have been largely indifferent as well, apart from the fact that only Firefox seems to have a perfectly-to-spec version of entry and incumbent.

I think the way to proceed is to perform some tests and come up with a comprehensive plan for changing everything to current or relevant. Then we can see how well that matches existing service worker implementations. In the best case, it just aligns with implementations better, so the implementers don't care. In the worst case, both implementations have already done incumbent or entry, so this would be a change for them. If that's true then we should bring it up with implementers, and possibly not make the change.

So I guess the first step is to write up tests for all the different uses.

[domenic]

I've verified by code inspection that Chrome and Firefox seem to agree on the various places that this spec and fetch use "entry", documented in whatwg/html#1431. Most of them are around URL parsing.

I think it would be ideal if implementers were willing to coordinate on a switch to the relevant settings object of the object on which the method is being called (or the current settings object in the case of constructors). This might require use counters if people think this change is risky; it seems very unlikely to be risky to me given how few developers do these sort of multi-global things, but maybe it's better to be safe than sorry. /cc @wanderview @mattto

What would be a good next step here? Would writing up a spec PR with the changes I propose be a good way to concretize this?

[jungkees]

I'd like to get feedback from @wanderview and @matto if they're willing to change. It seems SW and window.open() are the ones that have been implemented using entry as spec'd. I'm into this change as the goal is to minimize the use of entry concept. I also believe it'd be better to make other instances (e.g. window.open()) consistent with the base rule if the change is not risky. If the implementers agree, I can update the spec.

[jungkees]

@wanderview, navigator.serviceWorker.getRegistration(clientURL) in FF uses something like "incumbent's parent" rather than "entry" to parse the URL. Can you check whether it's a bug?

[mfalken]

What does entry and incumbent mean in easy-to-understand language?

[jungkees]

Entry is enteredExecutionContext() in Blink. I'm not sure what the corresponding function to get the incumbent context in Blink. /cc @jeisinger

Entry: https://html.spec.whatwg.org/multipage/webappapis.html#concept-entry-everything
Incumbent: https://html.spec.whatwg.org/multipage/webappapis.html#concept-incumbent-everything
The example in https://html.spec.whatwg.org/multipage/webappapis.html#realms-settings-objects-global-objects seems the most intuitive explanation I guess.

[mfalken]

Thanks. Maybe the best way forward is to write the WPTs that would be affected by the change, and then see what the current implementations do and whether we expect the changes to be risky.

Looking at the spec, the things that would be affected are:

• ExtendableMessageEvent and ServiceWorkerMessageEvent's .origin and .source
• navigate(url) and openWindow(url)
• registerForeignFetch's scope
  ?

[wanderview]

+1 to seeing the wpt test changes.

[domenic]

Awesome, thanks for being willing to work with us on this. I'll take the action item to write up some web platform tests.

[mkruisselbrink]

registerForeignFetch's scope ?

This one shouldn't really be effected since it's only callable from within a service worker, so there isn't more than one global you can reach that has that method. Also should be easy to change anyway since it hasn't shipped yet of course.

Similar I don't think there are any issues around navigate(url) and openWindow(url), as both of these methods are similarly only exposed within service workers, where as far as I can tell all four settings objects would always be the same.

[jeisinger]

Note that enteredExecutionContext returns the entry context, or the current context... so it's kinda random.

In fact, we don't have an entered context during microtask execution, so if you try to invoke ServiceWorkerContainer.register in a promise handler you'll get different semantics compared to when running it in a regular task :-/

[domenic]

In fact, we don't have an entered context during microtask execution

That's whatwg/html#1426 FYI

[jakearchibald]

This feels like v1, but feel free to change it.

[jakearchibald]

Pre F2F notes: Doesn't feel like there's much to discuss here. Just work to do.

[slightlyoff]

Is this really v1?

[annevk]

It affects all impl, so yes?

[domenic]

Spending a bit more time on this.

Entry

URL parsing would change:

• ServiceWorkerContainer.prototype.register
• ServiceWorkerContainer.prototype.getRegistration
• "Start Register" algorithm

No affect, since multiple globals cannot be present inside a service worker and these are only exposed there:

• Client.prototype.navigate
• Clients.prototype.openWindow
• InstallEvent.prototype.registerForeignFetch

Incumbent

No affect, since multiple globals cannot be present inside a service worker and these are only exposed there:

• Client.prototype.navigate
• InstallEvent.prototype.registerForeignFetch

postMessage (special case)

• For ServiceWorker.prototype.postMessage, we need to align with the changes in Use only the incumbent global in postMessage whatwg/html#1573, which make the incumbent global the only one. There were some follow-up editorial changes in Parse postMessage()'s targetOrigin argument whatwg/html#1584 and Tidy up the window.postMessage algorithm further whatwg/html#1586 which are worth copying too. Since there is no targetOrigin in ServiceWorker.prototype.postMessage, the issues HTML had mostly do not apply here. It remains the case that we need to keep incumbent for this method.
• For Client.prototype.postMessage, we can get rid of incumbent/entry since there cannot be multiple globals present

Todo:

• I will submit a pull request cleaning up all usages that have no normative effect, and aligning postMessage with HTML's
• I will then work on web platform tests for the two URL parsing changes, and also probably for the postMessage alignment

[domenic]

OK, here's the web platform tests for the three URL parsing cases that remain: web-platform-tests/wpt#3449

As you can see this requires some esoteric contortions to actually come up in practice.

(They are written to demand relevant, which I think is more correct, so they will fail in Chrome and Firefox.)

[jungkees]

b546c24 has been backported to V1: 1d7f497

[domenic]

As noted above, I uploaded the web platform tests. Have people had a chance to review them, and hopefully agree with me that the change is doable? Is there anything else we can do to move this issue forward?

[mfalken]

Thanks for the tests. I agree this change is unlikely to break sites, and it makes sense. I'm willing to change Chrome implementation to conform to these tests.

[wanderview]

Works for me. Sorry for the delay in responding. (I really wish there was a need-info system in github.)

Gecko bug:

https://bugzilla.mozilla.org/show_bug.cgi?id=1298819

@domenic are you going to land the tests in the WPT repo? Or we can import them and uplift to WPT when we make the code changes. Not sure on time frame, though.

[domenic]

@wanderview I guess I should get a reviewer to sign off on the tests? But they're ready to land if so.

If people haven't seen, I also sent a spec PR: #963

[domenic]

Can someone sign-off on the corresponding tests PR at web-platform-tests/wpt#3449 ?

[domenic]

Tests merged to WPT; I also filed a Blink issue at https://bugs.chromium.org/p/chromium/issues/detail?id=691008.

[domenic]

Also filed an Edge issue for good measure, although I don't know if they implemented the old spec or new spec: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/10902802/