Web Locks API: Request fails with ifAvailable option when the lock should be available |
|||||||||
Issue description
Steps to reproduce:
1. Open two tabs, Tab A and Tab B.
2. In Tab A, request 'lock1' and, and do not resolve the request. This should successfully acquire 'lock1':
navigator.locks.request('lock1', (lock) => {
console.log(lock)
if (lock) {
return new Promise((resolve, reject) => {
console.log('not resolving');
});
}
});
3. In Tab B, request 'lock2' and, and do not resolve the request. This should successfully acquire 'lock2':
navigator.locks.request('lock2', (lock) => {
console.log(lock)
if (lock) {
return new Promise((resolve, reject) => {
console.log('not resolving');
});
}
});
4. Close Tab B, which should release 'lock2'.
5. In Tab A Verify that 'lock2' is released by running 'await navigator.locks.query()'. Only 'lock1' should be held.
6. In Tab A request 'lock2' with the {ifAvailable: true} option:
navigator.locks.request('lock2', {ifAvailable: true}, (lock) => {
console.log(lock)
if (lock) {
return new Promise((resolve, reject) => {
console.log('not resolving');
});
}
});
Expected result:
'lock2' is available and logged to the console.
Actual result:
'lock2' is not available, and null is logged.
Note: The lock is successfully acquired if the '{ifAvailable: true}' option is not passed in Step 6.
,
May 8 2018
Huh, thanks - I'll take a peek.
,
May 8 2018
From code inspection, it looks like LockManager::ReleaseLock doesn't necessarily cause the shared_/exclusive_ sets to be rebuilt after state.EraseLock() returns true (dirty). If there's no pending request, they aren't rebuilt. Odds are the current tests miss this case because they use a pending request to determine that the lock has become available.
,
May 8 2018
,
May 8 2018
I got nosy before this bug was filed, and looked at the code, and these two sets jumped at me as a potential problem. I'm probably out of my depth here, but can't we always go by the golden source maps (requested_, held_) ? :)
,
May 8 2018
After we have reasonable confidence in the API (Origin Trials over, shipping approved) I plan to replace some of the guts with an O(1) / request implementation. If you're volunteering to listen, I'd be happy to discuss the design with you before implementing when the time comes.
,
May 8 2018
Ah.. as suspected I didn't think it through enough :) I'll just keep lurking on your mailing list as usual.
,
May 8 2018
Not at all! I haven't been forthcoming about the implementation strategy here. The current code has solid interfaces and the right architecture, but the algorithms & data structures are optimized towards iterating quickly for now.
,
May 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0e021fc6edcf6e71152f8959fa324d6cc0f5ce8d commit 0e021fc6edcf6e71152f8959fa324d6cc0f5ce8d Author: Joshua Bell <jsbell@chromium.org> Date: Wed May 09 18:46:36 2018 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/1050807 Reviewed-by: Victor Costan <pwnall@chromium.org> Commit-Queue: Joshua Bell <jsbell@chromium.org> Cr-Commit-Position: refs/heads/master@{#557257} [modify] https://crrev.com/0e021fc6edcf6e71152f8959fa324d6cc0f5ce8d/content/browser/locks/lock_manager.cc [modify] https://crrev.com/0e021fc6edcf6e71152f8959fa324d6cc0f5ce8d/third_party/WebKit/LayoutTests/http/tests/locks/ifAvailable.html
,
May 9 2018
Hey reporters: Can you verify in Canary (tomorrow's build)? Then we should be able to make the case to merge to 67.
,
May 10 2018
Verified that the issue does not reproduce on 68.0.3426.0.
,
May 10 2018
Requesting merge for 67. This is not a regression, but it's for a feature in "Origin Trial" and with this bug the feedback we're getting is compromised. Risk is low since the feature is not broadly enabled.
,
May 10 2018
This bug requires manual review: M67 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 10 2018
Pls apply appropriate OSs label. Thank you.
,
May 10 2018
,
May 10 2018
Approving merge to M67 branch 3396 based on comment #13 and per offline chat with jsbell@, the code change is low risk/tiny. Pls merge ASAP. Thank you.
,
May 10 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fad465c0123c6ebe656b49b5fe14ac8625e7e0fb commit fad465c0123c6ebe656b49b5fe14ac8625e7e0fb Author: Joshua Bell <jsbell@chromium.org> Date: Thu May 10 20:27:46 2018 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/1050807 Reviewed-by: Victor Costan <pwnall@chromium.org> Commit-Queue: Joshua Bell <jsbell@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#557257}(cherry picked from commit 0e021fc6edcf6e71152f8959fa324d6cc0f5ce8d) Reviewed-on: https://chromium-review.googlesource.com/1054351 Reviewed-by: Joshua Bell <jsbell@chromium.org> Cr-Commit-Position: refs/branch-heads/3396@{#555} Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428} [modify] https://crrev.com/fad465c0123c6ebe656b49b5fe14ac8625e7e0fb/content/browser/locks/lock_manager.cc [modify] https://crrev.com/fad465c0123c6ebe656b49b5fe14ac8625e7e0fb/third_party/WebKit/LayoutTests/http/tests/locks/ifAvailable.html |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by ohsnapit...@google.com
, May 8 2018