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

[web-locks] Verify release during navigation #20258

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jugglinmike
Copy link
Contributor

This test presumes that "termination" occurs during navigation.

This test presumes that "termination" occurs during navigation.
Copy link
Contributor

@pwnall pwnall left a comment

Choose a reason for hiding this comment

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

Thank you for the test!

Here are some suggestions for improvements. This test improves coverage, so LGTM as-is if you'd rather not iterate on it.

@@ -0,0 +1,51 @@
<!DOCTYPE html>
<html>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: <html> <head> and <body> are optional in HTML5. The document will parse fine without them.

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 will parse fine, but without a body, the script will have nowhere to place the iframe. I'll keep that tag and remove the others.

<script>
'use strict';
(async () => {
const [uuid, phase] = location.search.split('_');
Copy link
Contributor

Choose a reason for hiding this comment

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

This works, but is a bit less clear than explicit query params. You could parse those using

const url = new URL(self.location.href);
const lockName = url.searchParams.get('lock');
const state = url.searchParams.get('state');

}, '*');

if (phase === 'first') {
location.search = uuid + '_second';
Copy link
Contributor

Choose a reason for hiding this comment

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

This would become

url.searchParams.set('state', 'second');
self.location = url;

(async () => {
const [uuid, phase] = location.search.split('_');
const neverSettled = new Promise(() => {});
navigator.locks.request(uuid, () => neverSettled);
Copy link
Contributor

Choose a reason for hiding this comment

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

A more robust version:

let firstRequestGranted = false;
navigator.locks.request(lockName, () => {
  firstRequestGranted = true;
  return neverSettled;
}); 
let secondRequestGranted = false;
navigator.locks.request(lockName, () => {
  secondRequestGranted = true;
  return neverSettled;
}); 

This doesn't depend on navigator.locks.query, which may have its own bugs. The frame would postMessage {firstRequestGranted, secondRequestGranted}.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I spoke too soon. I think that the test would be more robust if we waited for the first request to be granted, then requested an unrelated lock and waited for that to be granted.

let firstRequestPromiseResolver;
const firstRequestPromise =
    new Promise(resolve => { firstRequestPromiseResolver = resolve; });
navigator.locks.request(lockName, () => {
  firstRequestPromiseResolver();
  return neverSettled;
});
await firstRequestPromise;
// This request should never get granted.
let secondRequestGranted = false;
navigator.locks.request(lockName, () => {
  secondRequestGranted = true;
  return neverSettled;
});
let lastRequestPromiseResolver;
// Give the lock manager a chance to grant locks.
const lastRequestPromise =
    new Promise(resolve => { lastRequestPromiseResolver = resolve; });
navigator.locks.request(token(), () => {
  lastRequestPromiseResolver();
  return neverSettled;
}); 
await lastRequestPromise;

self.parent.postMessage({ secondRequestGranted, state });

@jugglinmike
Copy link
Contributor Author

Thanks for the review, @pwnall! At the risk of starting a game of golf, I've iterated on your suggestion a bit.

This latest revision combines your initial idea with your amended idea: it uses two booleans and three requests. However, it only waits on the third request. My rationale is that the expected sequence is still verifiable in this form (thanks to the single FIFO lock task queue), but it reduces the number of failure modes that require timing out (e.g. if the UA incorrectly refuses to grant the named lock following navigation, then the test will complete immediately, but it will still fail because flag for that request will be false when the third request is granted).

Also, I fixed a race condition in the getMessage function. Though to be honest, it might still be racey--the child sends a message and immediately navigates, so the navigation may interfere with the message.

Copy link
Member

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

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

Sorry about the delay in reviewing...

queue.push(event.data);
});

async function getMessage(name) {
Copy link
Member

Choose a reason for hiding this comment

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

name is unused?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants