Verify service worker behavior with initial about:blank iframe docume… #6304
Conversation
…nts. DO NOT MERGE
Firefox (nightly)Testing web-platform-tests at revision dfbfa83 All results1 test ran/service-workers/service-worker/about-blank-replacement.https.html
|
Sauce (safari)Testing web-platform-tests at revision dfbfa83 All results1 test ran/service-workers/service-worker/about-blank-replacement.https.html
|
Chrome (unstable)Testing web-platform-tests at revision dfbfa83 All results1 test ran/service-workers/service-worker/about-blank-replacement.https.html
|
Sauce (MicrosoftEdge)Testing web-platform-tests at revision dfbfa83 All results1 test ran/service-workers/service-worker/about-blank-replacement.https.html
|
|
@jungkees @jakearchibald @mattto Do you have any objections to me landing this test in gecko and upstreaming it? I believe it matches our current understanding about initial about:blank handling. The test does not depend on any reserved or resultingClientId behavior. |
| // Helper routine that creates an iframe that internally has some kind | ||
| // of nested window. The nested window could be another iframe or | ||
| // it could be a popup window. | ||
| function createFrameWithNestedWindow(url) { |
There was a problem hiding this comment.
createIframeWithNestedWindow? Just to be specific that it's an iframe.
Nit: I think I'd have made this two functions. One to add the iframe (which I think we already have), and another to await the message. As it stands it feels like this function is doing two somewhat unrelated things, but maybe it's just me.
There was a problem hiding this comment.
The issue with that is with_frame() waits for the URL passed to it to load in the frame. But what we really want is to get the load event of the nested frame. Its unclear to me if the load events of the first frame and the nested frame are guaranteed. Just combining the operations together seemed safest and most likely to work in all browsers.
| parent.postMessage({ type: 'NESTED_LOADED' }, '*'); | ||
| } | ||
| </script> | ||
| <iframe src="?nested=true" id="nested" onload="nestedLoaded()"></iframe> |
There was a problem hiding this comment.
Won't this become recursive if the page isn't service worker controlled? As in, this iframe is pointing to the same URL, so it'll include itself etc etc.
Might be safer to point it at a blank page?
There was a problem hiding this comment.
The thing I struggle with there is that I need both the outer frame and the nested frame to be controlled under the same scope. Doing a query parameter was an easy way to accomplish that without creating a lot of extra sub-directories in the test. I suppose I could also use a server side to .py to return a blank document in that case.
| // inherit the controller from its parent. | ||
| async function getClientByURL(url) { | ||
| let list = await clients.matchAll(); | ||
| for (client of list) { |
There was a problem hiding this comment.
const client of list
There was a problem hiding this comment.
fwiw, this could be:
return list.find(client => client.url === url);|
|
||
| // Delay the load of the nested frame so we can examine the Client | ||
| // representing the window while it has the initial about:blank document. | ||
| evt.respondWith(delayedResponse()); |
There was a problem hiding this comment.
Does resolveDelayed need to be a global here? As far as I can tell, it doesn't need to be scoped beyond this fetch callback.
Could it be written as an async function, to make it read more sequentially? Eg:
evt.respondWith(async function() {
const client = await getClientByURL('about:blank');
if (!client) return new Response('failure: could not find about:blank client');
if (url.searchParams.get('ping')) {
const loc = await pingPong(client);
if (loc !== 'about:blank') {
return new Response(`failure: got location {$loc}, expected about:blank`);
}
}
return new Response(client.id);
}());There was a problem hiding this comment.
Yea, the async function is much cleaner. Thanks!
| // Execute a test where the nested frame is simple loaded normally. | ||
| await doAsyncTest(t, 'resources/about-blank-replacement-frame.html'); | ||
| }, 'Initial about:blank is controlled, exposed to clients.matchAll(), and ' + | ||
| 'matches final Client.'); |
There was a problem hiding this comment.
I think this would fail in Edge, since it reports the iframe's URL as its src. Test: https://iframe-global-retain.glitch.me/typical.html.
My worry about these tests is we really want the clients API to reflect what the browser does, more than what the standards say they should do. My worry is that browsers monkey-patch this for the clients API, but don't fix their underlying implementation.
Maybe it's worth a note at the top of this test?
There was a problem hiding this comment.
I agree clients should reflect what the browser is doing, but if the browser doesn't create initial about:blank documents at all then maybe they should just mark this test as expected fail until they fix that.
I'll add a comment, though.
| parent.postMessage({ type: 'NESTED_LOADED' }, '*'); | ||
| } | ||
| </script> | ||
| <iframe src="?nested=true&ping=true" id="nested" onload="nestedLoaded()"></iframe> |
There was a problem hiding this comment.
Same recursion issue here. Also & to &.
| parent.postMessage({ type: 'NESTED_LOADED' }, '*'); | ||
| } | ||
|
|
||
| let popup = window.open('?nested=true'); |
There was a problem hiding this comment.
Same recursion case.
|
I'm also going to add a test case for w3c/ServiceWorker#1232. At a minimum we should verify we do not end up with a controlled iframe when there is no service worker matching the scope. |
|
The last commit addresses review feedback, but does not add the new test case yet. |
…:blank can end up uncontrolled based on its load URL.
|
@jakearchibald, do you mind reviewing the additional test case I added? I did not verify whether or not the global is shared. I only verified that there is an initial about:blank and that the final load is not controlled. I think this gives us flexibility in w3c/ServiceWorker#1232. I also think its the minimum requirement for this to make any sense. |
Build PASSEDStarted: 2017-11-27 22:20:43 View more information about this build on: |
| // Helper routine that creates an iframe that internally has some kind | ||
| // of nested window. The nested window could be another iframe or | ||
| // it could be a popup window. | ||
| function createFrameWithNestedWindow(url) { |
|
Thanks! I realized I forgot to do the Also, do we squash commits before merging here? Or I might land it in gecko and upstream. |
| return; | ||
| } | ||
| window.removeEventListener('message', onMsg); | ||
| if(evt.data.result && evt.data.result.startsWith('failure:')) { |
| } | ||
|
|
||
| promise_test(async function(t) { | ||
| // Execute a test where the nested frame is simple loaded normally. |
…nts.