Web Locks API: Ensure shared/exclusive sets are rebuilt after a release

The code contained an optimization to skip rebuilding sets of held
shared/granted locks if there was no pending request. This was not
valid, since a request could come in later and rely on the sets to
correctly determine if the lock was grantable.

Bug: 840994
Change-Id: I64218cfaebe4ff7b46f133665a94378c66d0e738
Reviewed-on: https://chromium-review.googlesource.com/1050807Reviewed-by: Victor Costan <pwnall@chromium.org>
Commit-Queue: Joshua Bell <jsbell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557257}

content/browser/locks/lock_manager.cc

@@ -206,9 +206,6 @@ class LockManager::OriginState {
   }
 
   void ProcessRequests(LockManager* lock_manager, const url::Origin& origin) {
-    if (requested_.empty())
-      return;
-
     shared_.clear();
     exclusive_.clear();
     for (const auto& id_lock_pair : held_) {

third_party/WebKit/LayoutTests/http/tests/locks/ifAvailable.html

@@ -4,12 +4,14 @@
 <link rel=help href="https://github.com/inexorabletash/web-locks">
 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
+<script src="resources/helpers.js"></script>
 <script>
 'use strict';
 
 promise_test(async t => {
+  const res = self.uniqueName(t);
   let callback_called = false;
-  await navigator.locks.request('resource', {ifAvailable: true}, async lock => {
+  await navigator.locks.request(res, {ifAvailable: true}, async lock => {
     callback_called = true;
     assert_not_equals(lock, null, 'lock should be granted');
   });
@@ -17,11 +19,12 @@ promise_test(async t => {
 }, 'Lock request with ifAvailable - lock available');
 
 promise_test(async t => {
+  const res = self.uniqueName(t);
   let callback_called = false;
-  await navigator.locks.request('resource', async lock => {
+  await navigator.locks.request(res, async lock => {
     // Request would time out if |ifAvailable| was not specified.
     const result = await navigator.locks.request(
-      'resource', {ifAvailable: true}, async lock => {
+      res, {ifAvailable: true}, async lock => {
         callback_called = true;
         assert_equals(lock, null, 'lock should not be granted');
         return 123;
@@ -32,11 +35,12 @@ promise_test(async t => {
 }, 'Lock request with ifAvailable - lock not available');
 
 promise_test(async t => {
+  const res = self.uniqueName(t);
   let callback_called = false;
-  await navigator.locks.request('resource', async lock => {
+  await navigator.locks.request(res, async lock => {
     try {
       // Request would time out if |ifAvailable| was not specified.
-      await navigator.locks.request('resource', {ifAvailable: true}, async lock => {
+      await navigator.locks.request(res, {ifAvailable: true}, async lock => {
         callback_called = true;
         assert_equals(lock, null, 'lock should not be granted');
         throw 123;
@@ -50,8 +54,9 @@ promise_test(async t => {
 }, 'Lock request with ifAvailable - lock not available, callback throws');
 
 promise_test(async t => {
+  const res = self.uniqueName(t);
   let callback_called = false;
-  await navigator.locks.request('resource', async lock => {
+  await navigator.locks.request(res, async lock => {
     // Request with a different name - should be grantable.
     await navigator.locks.request('different', {ifAvailable: true}, async lock => {
       callback_called = true;
@@ -62,10 +67,11 @@ promise_test(async t => {
 }, 'Lock request with ifAvailable - unrelated lock held');
 
 promise_test(async t => {
+  const res = self.uniqueName(t);
   let callback_called = false;
-  await navigator.locks.request('resource', {mode: 'shared'}, async lock => {
+  await navigator.locks.request(res, {mode: 'shared'}, async lock => {
     await navigator.locks.request(
-      'resource', {mode: 'shared', ifAvailable: true}, async lock => {
+      res, {mode: 'shared', ifAvailable: true}, async lock => {
         callback_called = true;
         assert_not_equals(lock, null, 'lock should be granted');
       });
@@ -74,10 +80,11 @@ promise_test(async t => {
 }, 'Shared lock request with ifAvailable - shared lock held');
 
 promise_test(async t => {
+  const res = self.uniqueName(t);
   let callback_called = false;
-  await navigator.locks.request('resource', {mode: 'shared'}, async lock => {
+  await navigator.locks.request(res, {mode: 'shared'}, async lock => {
     // Request would time out if |ifAvailable| was not specified.
-    await navigator.locks.request('resource', {ifAvailable: true}, async lock => {
+    await navigator.locks.request(res, {ifAvailable: true}, async lock => {
       callback_called = true;
       assert_equals(lock, null, 'lock should not be granted');
     });
@@ -86,11 +93,12 @@ promise_test(async t => {
 }, 'Exclusive lock request with ifAvailable - shared lock held');
 
 promise_test(async t => {
+  const res = self.uniqueName(t);
   let callback_called = false;
-  await navigator.locks.request('resource', async lock => {
+  await navigator.locks.request(res, async lock => {
     // Request would time out if |ifAvailable| was not specified.
     await navigator.locks.request(
-      'resource', {mode: 'shared', ifAvailable: true}, async lock => {
+      res, {mode: 'shared', ifAvailable: true}, async lock => {
         callback_called = true;
         assert_equals(lock, null, 'lock should not be granted');
       });
@@ -99,12 +107,13 @@ promise_test(async t => {
 }, 'Shared lock request with ifAvailable - exclusive lock held');
 
 promise_test(async t => {
+  const res = self.uniqueName(t);
   let callback_called = false;
-  await navigator.locks.request('resource', async lock => {
+  await navigator.locks.request(res, async lock => {
     callback_called = true;
     const test_error = {name: 'test'};
     const p = navigator.locks.request(
-      'resource', {ifAvailable: true}, lock => {
+      res, {ifAvailable: true}, lock => {
         assert_equals(lock, null, 'lock should not be available');
         throw test_error;
       });
@@ -115,12 +124,13 @@ promise_test(async t => {
 }, 'Returned Promise rejects if callback throws synchronously');
 
 promise_test(async t => {
+  const res = self.uniqueName(t);
   let callback_called = false;
-  await navigator.locks.request('resource', async lock => {
+  await navigator.locks.request(res, async lock => {
     callback_called = true;
     const test_error = {name: 'test'};
     const p = navigator.locks.request(
-      'resource', {ifAvailable: true}, async lock => {
+      res, {ifAvailable: true}, async lock => {
         assert_equals(lock, null, 'lock should not be available');
         throw test_error;
       });
@@ -130,4 +140,30 @@ promise_test(async t => {
   assert_true(callback_called, 'callback should be called');
 }, 'Returned Promise rejects if async callback yields rejected promise');
 
+// Regression test for: https://crbug.com/840994
+promise_test(async t => {
+  const res1 = self.uniqueName(t);
+  const res2 = self.uniqueName(t);
+  let callback1_called = false;
+  await navigator.locks.request(res1, async lock => {
+    callback1_called = true;
+    let callback2_called = false;
+    await navigator.locks.request(res2, async lock => {
+      callback2_called = true;
+    });
+    assert_true(callback2_called, 'callback2 should be called');
+
+    let callback3_called = false;
+    await navigator.locks.request(res2, {ifAvailable: true}, async lock => {
+      callback3_called = true;
+      // This request would fail if the "is this grantable?" test
+      // failed, e.g. due to the release without a pending request
+      // skipping steps.
+      assert_not_equals(lock, null, 'Lock should be available');
+    });
+    assert_true(callback3_called, 'callback2 should be called');
+  });
+  assert_true(callback1_called, 'callback1 should be called');
+}, 'Locks are available once previous release is processed');
+
 </script>