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.

Sign up for GitHub

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

Jump to bottom

BroadcastChannel: Ignore messages from detached iframes / closing workers #7296

Merged
merged 2 commits into from
Nov 5, 2021
Merged

BroadcastChannel: Ignore messages from detached iframes / closing workers #7296

merged 2 commits into from
Nov 5, 2021

Conversation

recvfrom
Copy link
Contributor

@recvfrom recvfrom commented Nov 4, 2021 •
edited by pr-preview bot
Loading

For BroadcastChannel instances associated with detached iframes or closing workers, update the specification to mention that calls to postMessage should be ignored without throwing an exception.

This update should align with the consensus from #7219 regarding how to handle these two cases.

/acknowledgements.html ( diff )
/web-messaging.html ( diff )

@recvfrom
BroadcastChannel: Ignore msgs from detached iframes / closing workers
c147158 
For BroadcastChannel instances associated with detached iframes
or closing workers, update the specification to mention that
calls to postMessage should be ignored without throwing an
exception.
domenic
domenic approved these changes Nov 4, 2021
View reviewed changes
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

LGTM with nit. This also makes calling postMessage() while in bfcache a no-op, but I think that is not really observable today so it should be fine and not require any additional tests.

(Details: The only way I can think of to call postMessage() from a bfcached page is to get a reference to a BroadcastChannel from that global before the page enters bfcache, then after it enters bfcache call broadcastChannel.postMessage() "from the outside". However, browsers don't let you bfcache iframes or windows with an opener today, from what I understand. So there is no way to get such a reference.)

source Show resolved Hide resolved
@domenic
Copy link
Member

domenic commented Nov 4, 2021

Also once this is merged (or even before since it looks like we have consensus) please remember to remove the .tentative from the relevant test filenames.

@recvfrom
Copy link
Contributor Author

recvfrom commented Nov 4, 2021

Thanks for the review, @domenic. I'll submit a CL today that removes tentative from the test file names.

@domenic
Copy link
Member

domenic commented Nov 4, 2021

Awesome! I will give this ~24 hours in case anyone from #7219 wants to take a look (/cc @wanderview @asutherland @cdumez) but it looks great. Special congrats on this being your first contribution; it's very clean!

@asutherland
Copy link

I also like this very much, thank you!

@chromium-wpt-export-bot chromium-wpt-export-bot mentioned this pull request Nov 5, 2021
Merged
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 5, 2021
@recvfrom @chromium-wpt-export-bot
BroadcastChannel: Make detached iframe / closed worker tests permanent
20850dd 
The spec is being updated [1] to indicate that messages from
BroadcastChannel instances associated with detached iframe /
closed worker contexts should be ignored, which means that
these tests no longer need to be marked 'tentative'.

Note that this CL doesn't introduce any changes to the tests.

[1] whatwg/html#7296

Bug: 1239274
Change-Id: I3e506493175b5e22913c35510454e8bef5f1e5e8
@domenic domenic merged commit f8cb0cc into whatwg:main Nov 5, 2021
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 5, 2021
@recvfrom @chromium-wpt-export-bot
BroadcastChannel: Make detached iframe / closed worker tests permanent
f9fa23f 
The spec is being updated [1] to indicate that messages from
BroadcastChannel instances associated with detached iframe /
closed worker contexts should be ignored, which means that
these tests no longer need to be marked 'tentative'.

Note that this CL doesn't introduce any changes to the tests.

[1] whatwg/html#7296

Bug: 1239274
Change-Id: I3e506493175b5e22913c35510454e8bef5f1e5e8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3259423
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Auto-Submit: Andrew Williams <awillia@google.com>
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/main@{#938781}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 5, 2021
@recvfrom @chromium-wpt-export-bot
BroadcastChannel: Make detached iframe / closed worker tests permanent
74a7f0a 
The spec is being updated [1] to indicate that messages from
BroadcastChannel instances associated with detached iframe /
closed worker contexts should be ignored, which means that
these tests no longer need to be marked 'tentative'.

Note that this CL doesn't introduce any changes to the tests.

[1] whatwg/html#7296

Bug: 1239274
Change-Id: I3e506493175b5e22913c35510454e8bef5f1e5e8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3259423
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Auto-Submit: Andrew Williams <awillia@google.com>
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/main@{#938781}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 10, 2021
@recvfrom @moz-wptsync-bot
Bug 1739544 [wpt PR 31513] - BroadcastChannel: Make detached iframe /…
b1c28ce 
… closed worker tests permanent, a=testonly

Automatic update from web-platform-tests
BroadcastChannel: Make detached iframe / closed worker tests permanent

The spec is being updated [1] to indicate that messages from
BroadcastChannel instances associated with detached iframe /
closed worker contexts should be ignored, which means that
these tests no longer need to be marked 'tentative'.

Note that this CL doesn't introduce any changes to the tests.

[1] whatwg/html#7296

Bug: 1239274
Change-Id: I3e506493175b5e22913c35510454e8bef5f1e5e8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3259423
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Auto-Submit: Andrew Williams <awillia@google.com>
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/main@{#938781}

--

wpt-commits: 74a7f0ac9da8a098c16e421de381d5bcc1c3b5f7
wpt-pr: 31513
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Nov 10, 2021
@recvfrom @moz-wptsync-bot
Bug 1739544 [wpt PR 31513] - BroadcastChannel: Make detached iframe /…
60a4a14 
… closed worker tests permanent, a=testonly

Automatic update from web-platform-tests
BroadcastChannel: Make detached iframe / closed worker tests permanent

The spec is being updated [1] to indicate that messages from
BroadcastChannel instances associated with detached iframe /
closed worker contexts should be ignored, which means that
these tests no longer need to be marked 'tentative'.

Note that this CL doesn't introduce any changes to the tests.

[1] whatwg/html#7296

Bug: 1239274
Change-Id: I3e506493175b5e22913c35510454e8bef5f1e5e8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3259423
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Auto-Submit: Andrew Williams <awillia@google.com>
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/main@{#938781}

--

wpt-commits: 74a7f0ac9da8a098c16e421de381d5bcc1c3b5f7
wpt-pr: 31513
Gabisampaio pushed a commit to Gabisampaio/wpt that referenced this pull request Nov 18, 2021
@recvfrom @Gabisampaio
BroadcastChannel: Make detached iframe / closed worker tests permanent
f5ddc00 
The spec is being updated [1] to indicate that messages from
BroadcastChannel instances associated with detached iframe /
closed worker contexts should be ignored, which means that
these tests no longer need to be marked 'tentative'.

Note that this CL doesn't introduce any changes to the tests.

[1] whatwg/html#7296

Bug: 1239274
Change-Id: I3e506493175b5e22913c35510454e8bef5f1e5e8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3259423
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Auto-Submit: Andrew Williams <awillia@google.com>
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/main@{#938781}
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
@recvfrom @copybara-github
BroadcastChannel: Make detached iframe / closed worker tests permanent
7e0ae11 
The spec is being updated [1] to indicate that messages from
BroadcastChannel instances associated with detached iframe /
closed worker contexts should be ignored, which means that
these tests no longer need to be marked 'tentative'.

Note that this CL doesn't introduce any changes to the tests.

[1] whatwg/html#7296

Bug: 1239274
Change-Id: I3e506493175b5e22913c35510454e8bef5f1e5e8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3259423
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Auto-Submit: Andrew Williams <awillia@google.com>
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/main@{#938781}
NOKEYCHECK=True
GitOrigin-RevId: f49494ed94e1a04b376707b3858c7c2b970e1148
@fergald fergald mentioned this pull request Jun 1, 2023
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pann-ux Pann-ux Pann-ux left review comments

@domenic domenic domenic approved these changes

Assignees
No one assigned
Labels
normative change topic: document lifecycle topic: serialize and transfer topic: workers
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@recvfrom @domenic @asutherland @Pann-ux