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

Verify service worker behavior with initial about:blank iframe docume… #6304

Merged
merged 5 commits into from
Nov 27, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 102 additions & 0 deletions service-workers/service-worker/about-blank-replacement.https.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
<!DOCTYPE html>
<title>Service Worker: about:blank replacement handling</title>
<meta name=timeout content=long>
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<script src="/common/get-host-info.sub.js"></script>
<script src="resources/test-helpers.sub.js"></script>
<body>
<script>
const worker = 'resources/about-blank-replacement-worker.js';

// 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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough!

return new Promise(resolve => {
let frame = document.createElement('iframe');
frame.src = url;
document.body.appendChild(frame);

window.addEventListener('message', function onMsg(evt) {
if (evt.data.type !== 'NESTED_LOADED') {
return;
}
window.removeEventListener('message', onMsg);
resolve(frame);
});
});
}

// Helper routine to request the given worker find the client with
// the specified URL using the clients.matchAll() API.
function getClientIdByURL(worker, url) {
return new Promise(resolve => {
navigator.serviceWorker.addEventListener('message', function onMsg(evt) {
if (evt.data.type !== 'GET_CLIENT_ID') {
return;
}
navigator.serviceWorker.removeEventListener('message', onMsg);
resolve(evt.data.result);
});
worker.postMessage({ type: 'GET_CLIENT_ID', url: url.toString() });
});
}

async function doAsyncTest(t, scope, extraSearchParams) {
let reg = await service_worker_unregister_and_register(t, worker, scope);
await wait_for_state(t, reg.installing, 'activated');

// Load the scope as a frame. We expect this in turn to have a nested
// iframe. The service worker will intercept the load of the nested
// iframe and populate its body with the client ID of the initial
// about:blank document it sees via clients.matchAll().
let frame = await createFrameWithNestedWindow(scope);
let initialResult = frame.contentWindow.nested().document.body.textContent;
assert_false(initialResult.startsWith('failure:'), `result: ${initialResult}`);

// Next, ask the service worker to find the final client ID for the fully
// loaded nested frame.
let nestedURL = new URL(scope, window.location);
nestedURL.searchParams.set('nested', true);
extraSearchParams = extraSearchParams || {};
for (let p in extraSearchParams) {
nestedURL.searchParams.set(p, extraSearchParams[p]);
}
let finalResult = await getClientIdByURL(reg.active, nestedURL);
assert_false(finalResult.startsWith('failure:'), `result: ${finalResult}`);

// The initial about:blank client and the final loaded client should have
// the same ID value.
assert_equals(initialResult, finalResult, 'client ID values should match');

frame.remove();
await service_worker_unregister_and_done(t, scope);
}

promise_test(async function(t) {
// Execute a test where the nested frame is simple loaded normally.
Copy link
Member

Choose a reason for hiding this comment

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

simply loaded normally?

await doAsyncTest(t, 'resources/about-blank-replacement-frame.html');
}, 'Initial about:blank is controlled, exposed to clients.matchAll(), and ' +
'matches final Client.');
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


promise_test(async function(t) {
// Execute a test where the nested frame is modified immediately by
// its parent. In this case we add a message listener so the service
// worker can ping the client to verify its existence. This ping-pong
// check is performed during the initial load and when verifying the
// final loaded client.
await doAsyncTest(t, 'resources/about-blank-replacement-ping-frame.html',
{ 'ping': true });
}, 'Initial about:blank modified by parent is controlled, exposed to ' +
'clients.matchAll(), and matches final Client.');

promise_test(async function(t) {
// Execute a test where the nested window is a popup window instead of
// an iframe. This should behave the same as the simple iframe case.
await doAsyncTest(t, 'resources/about-blank-replacement-popup-frame.html');
}, 'Popup initial about:blank is controlled, exposed to clients.matchAll(), and ' +
'matches final Client.');

</script>
</body>
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
<!doctype html>
<html>
<body>
<script>
function nestedLoaded() {
parent.postMessage({ type: 'NESTED_LOADED' }, '*');
}
</script>
<iframe src="?nested=true" id="nested" onload="nestedLoaded()"></iframe>
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

<script>
// Helper routine to make it slightly easier for our parent to find
// the nested frame.
function nested() {
return document.getElementById('nested').contentWindow;
}

// NOTE: Make sure not to touch the iframe directly here. We want to
// test the case where the initial about:blank document is not
// directly accessed before load.
</script>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<!doctype html>
<html>
<body>
<script>
function nestedLoaded() {
parent.postMessage({ type: 'NESTED_LOADED' }, '*');
}
</script>
<iframe src="?nested=true&ping=true" id="nested" onload="nestedLoaded()"></iframe>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same recursion issue here. Also & to &amp;.

<script>
// Helper routine to make it slightly easier for our parent to find
// the nested frame.
function nested() {
return document.getElementById('nested').contentWindow;
}

// This modifies the nested iframe immediately and does not wait for it to
// load. This effectively modifies the global for the initial about:blank
// document. Any modifications made here should be preserved after the
// frame loads because the global should be re-used.
let win = nested();
win.navigator.serviceWorker.addEventListener('message', evt => {
if (evt.data.type === 'PING') {
evt.source.postMessage({
type: 'PONG',
location: win.location.toString()
});
}
});
</script>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<!doctype html>
<html>
<body>
<script>
function nestedLoaded() {
parent.postMessage({ type: 'NESTED_LOADED' }, '*');
}

let popup = window.open('?nested=true');
Copy link
Contributor

Choose a reason for hiding this comment

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

Same recursion case.

popup.onload = nestedLoaded;

// Helper routine to make it slightly easier for our parent to find
// the nested popup window.
function nested() {
return popup;
}
</script>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
// Helper routine to delay the nested frame load until we are done interacting
// with the initial about:blank client.
let resolveDelayed;
function delayedResponse() {
return new Promise(resolve => {
resolveDelayed = resolve;
});
}

// Helper routine to find a client that matches a particular URL. Note, we
// require that Client to be controlled to avoid false matches with other
// about:blank windows the browser might have. The initial about:blank should
// inherit the controller from its parent.
async function getClientByURL(url) {
let list = await clients.matchAll();
for (client of list) {
Copy link
Contributor

Choose a reason for hiding this comment

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

const client of list

Copy link
Contributor

Choose a reason for hiding this comment

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

fwiw, this could be:

return list.find(client => client.url === url);

if (client.url === url) {
return client;
}
}
}

// Helper routine to perform a ping-pong with the given target client. We
// expect the Client to respond with its location URL.
async function pingPong(target) {
function waitForPong() {
return new Promise(resolve => {
self.addEventListener('message', function onMessage(evt) {
if (evt.data.type === 'PONG') {
resolve(evt.data.location);
}
});
});
}

target.postMessage({ type: 'PING' })
return await waitForPong(target);
}

addEventListener('fetch', async evt => {
let url = new URL(evt.request.url);
if (!url.searchParams.get('nested')) {
return;
}

// 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());
Copy link
Contributor

Choose a reason for hiding this comment

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

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);
}());

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, the async function is much cleaner. Thanks!


// Find the initial about:blank document.
let client = await getClientByURL('about:blank');
if (!client) {
resolveDelayed(new Response('failure: could not find about:blank client'));
return;
}

// If the nested frame is configured to support a ping-pong, then
// ping it now to verify its message listener exists. We also
// verify the Client's idea of its own location URL while we are doing
// this.
if (url.searchParams.get('ping')) {
let loc = await pingPong(client);
if (loc !== 'about:blank') {
resolveDelayed(new Response(`failure: got location {$loc}, expected about:blank`));
return;
}
}

// Finally, allow the nested frame to complete loading. We place the
// Client ID we found for the initial about:blank in the body.
resolveDelayed(new Response(client.id));
});

addEventListener('message', evt => {
if (evt.data.type !== 'GET_CLIENT_ID') {
return;
}

evt.waitUntil(async function() {
let url = new URL(evt.data.url);

// Find the given Client by its URL.
let client = await getClientByURL(evt.data.url);
if (!client) {
evt.source.postMessage({
type: 'GET_CLIENT_ID',
result: `failure: could not find ${evt.data.url} client`
});
return;
}

// If the Client supports a ping-pong, then do it now to verify
// the message listener exists and its location matches the
// Client object.
if (url.searchParams.get('ping')) {
let loc = await pingPong(client);
if (loc !== evt.data.url) {
evt.source.postMessage({
type: 'GET_CLIENT_ID',
result: `failure: got location ${loc}, expected ${evt.data.url}`
});
return;
}
}

// Finally, send the client ID back.
evt.source.postMessage({
type: 'GET_CLIENT_ID',
result: client.id
});
}());
});