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

Parallelism and task sources #202

Open
evanstade opened this issue Dec 12, 2023 · 2 comments
Open

Parallelism and task sources #202

evanstade opened this issue Dec 12, 2023 · 2 comments

Comments

@evanstade
Copy link

A few related comments:

  • the read() algorithm verifies permissions "in parallel", which is, AFAIU, a deprecated way to describe parallelism. Presumably this should be done on the permissions task source, so the "in parallel" should instead be a task queued on the permission task source.
  • the reference to the permission task source should link to it
  • I suspect the "Let data be a copy of the system clipboard data" should also be run on the clipboard task source, instead of just "in parallel", so as to prevent races (since write() place data on the system clipboard on the clipboard task source).
  • The spec says "The clipboard task source is triggered in response to reading or writing of system clipboard data." What does "is triggered in response to" mean? The idiomatic verbiage seems to be "The foo task source is a task source used to perform foo-related tasks".
@saschanaz
Copy link
Member

  • the read() algorithm verifies permissions "in parallel", which is, AFAIU, a deprecated way to describe parallelism.

Is there a documentation regarding to this? I heard that "Return promise and run the remaining steps in parallel" pattern is deprecated but never heard that "in parallel" itself is, that's new to me 👀

@evanstade
Copy link
Author

evanstade commented Dec 12, 2023

Perhaps "deprecated" is too strong a word. "Unfavored"? "overused"? "Rarely the best choice"? It seems that "in parallel" is often an under-specification because it introduces races into any algorithm steps that operate on global state, and is unnecessary in the case of steps that operate on local state in the context of a returned promise. It was applied more widely in older specs. What few specs I have written have gotten by with a parallel queue, task source, or the use of promises.

In the case of the clipboard, the problem introduced by this "in parallel" is that "check clipboard read permission" should theoretically always return false, because the step "Return true if the current script is running as a result of user interaction with a "Paste" element created by the user agent or operating system." will always fail, at least under my interpretation of "the current script". It is also racy given that it checks user activation, but "in parallel" steps may execute after transient activation has terminated.

Also it now occurs to me that the read() algorithm should just not reference the permission task source at all because it doesn't actually check any permission at the moment.

aarongable pushed a commit to chromium/chromium that referenced this issue Dec 12, 2023
This effectively gets rid of the "in parallel" part of the
spec algorithms that say "create a promise, then in parallel".
This parallelism isn't necessary and introduces races.
See w3c/clipboard-apis#202

Example of a fixed race: the check that the renderer is focused.
This check could have failed if the site lost focus very shortly
after using the clipboard (albeit not a very catastrophic outcome).
It also has the effect of eliminating an extra copy of clipboard
data, which could save a lot of memory depending on the size of the
payload.

This change also introduces a task source for the clipboard
as mentioned in
https://www.w3.org/TR/clipboard-apis/#clipboard-task-source

This task source is useful for making sure multiple clipboard
operations (reading, writing to the system clipboard) are not
occurring in parallel. Other steps, like when an error condition
causes a promise to be rejected, need not be executed on that same
task source. Note that the spec currently fails to describe many
of the error conditions that Chromium implements. Also note that
Chromium's implementation fails to group steps that should
be part of a single operation into a single task in the queue.
See crbug.com/1510722

Bug: 1510722,1501971
Change-Id: I2eb7e8ca61bd3f7f1830514c8a68b3f1ddec042a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5113849
Reviewed-by: Anupam Snigdha <[email protected]>
Reviewed-by: Daniel Cheng <[email protected]>
Commit-Queue: Evan Stade <[email protected]>
Reviewed-by: Scott Haseley <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1236575}
github-actions bot pushed a commit to kaidokert/chrome_base_mirror that referenced this issue Dec 13, 2023
This effectively gets rid of the "in parallel" part of the
spec algorithms that say "create a promise, then in parallel".
This parallelism isn't necessary and introduces races.
See w3c/clipboard-apis#202

Example of a fixed race: the check that the renderer is focused.
This check could have failed if the site lost focus very shortly
after using the clipboard (albeit not a very catastrophic outcome).
It also has the effect of eliminating an extra copy of clipboard
data, which could save a lot of memory depending on the size of the
payload.

This change also introduces a task source for the clipboard
as mentioned in
https://www.w3.org/TR/clipboard-apis/#clipboard-task-source

This task source is useful for making sure multiple clipboard
operations (reading, writing to the system clipboard) are not
occurring in parallel. Other steps, like when an error condition
causes a promise to be rejected, need not be executed on that same
task source. Note that the spec currently fails to describe many
of the error conditions that Chromium implements. Also note that
Chromium's implementation fails to group steps that should
be part of a single operation into a single task in the queue.
See crbug.com/1510722

Bug: 1510722,1501971
Change-Id: I2eb7e8ca61bd3f7f1830514c8a68b3f1ddec042a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5113849
Reviewed-by: Anupam Snigdha <[email protected]>
Reviewed-by: Daniel Cheng <[email protected]>
Commit-Queue: Evan Stade <[email protected]>
Reviewed-by: Scott Haseley <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1236575}
NOKEYCHECK=True
GitOrigin-RevId: 35ff80a930b42b4aa25bf8b0e8624448c9c34f35
jrguzman-ms pushed a commit to msft-mirror-aosp/platform.external.libchrome that referenced this issue Dec 15, 2023
This effectively gets rid of the "in parallel" part of the
spec algorithms that say "create a promise, then in parallel".
This parallelism isn't necessary and introduces races.
See w3c/clipboard-apis#202

Example of a fixed race: the check that the renderer is focused.
This check could have failed if the site lost focus very shortly
after using the clipboard (albeit not a very catastrophic outcome).
It also has the effect of eliminating an extra copy of clipboard
data, which could save a lot of memory depending on the size of the
payload.

This change also introduces a task source for the clipboard
as mentioned in
https://www.w3.org/TR/clipboard-apis/#clipboard-task-source

This task source is useful for making sure multiple clipboard
operations (reading, writing to the system clipboard) are not
occurring in parallel. Other steps, like when an error condition
causes a promise to be rejected, need not be executed on that same
task source. Note that the spec currently fails to describe many
of the error conditions that Chromium implements. Also note that
Chromium's implementation fails to group steps that should
be part of a single operation into a single task in the queue.
See crbug.com/1510722

Bug: 1510722,1501971
Change-Id: I2eb7e8ca61bd3f7f1830514c8a68b3f1ddec042a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5113849
Reviewed-by: Anupam Snigdha <[email protected]>
Reviewed-by: Daniel Cheng <[email protected]>
Commit-Queue: Evan Stade <[email protected]>
Reviewed-by: Scott Haseley <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1236575}


CrOS-Libchrome-Original-Commit: 35ff80a930b42b4aa25bf8b0e8624448c9c34f35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants