-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
creating a BroadcastChannel in a detached iframe #7219
Comments
In general I'm pretty supportive of making less things work in non-fully active documents if we can get away with it. (Note: this includes both detached iframes, and bfcached pages. /cc @rakina) I tested on Safari tech preview and they seem to close the I also tested a variant that moves So I see several possible behaviors on the table:
Safari TP behavior seems pretty reasonable to me, although I haven't tested to see if they're consistent between bfcache and detached iframes. |
Maybe we could also take this opportunity to make BroadcastChannel work less after (It currently works now because closing just means that all existing tasks get discarded and no new tasks can be queued but content logic can continue running for a while and create as many edge cases as possible.) |
Applying the same restrictions to closing workers, sounds good to me. |
Currently in Chrome we only bfcache a page with BroadcastChannel if it's closed. If we can standardize auto-closing existing BroadcastChannels (which seems like is what Safari TP is doing?) that would be nice. cc @fergald @rubberyuzu |
Do you mean, that the BroadcastChannel would get closed when entering the bfcache and stay closed when restoring from the bfcache? I doubt this is STP’s behavior and if it is then it would be a bug. BroadcastChannel should stay usable upon restoring from bfcache. |
We do not close the BroadcastChannel upon navigation. |
I like the STP behavior from #7219 (comment). @asutherland, is that something mozilla would support switching to? |
We should figure out a desired behavior for bfcache before we spec anything concrete... I didn't test Safari TP for that. I see the following options:
@cdumez @rubberyuzu any thoughts on the best path here? Or if there's another option I didn't think of? The reason this impacts speccing the detached frame case is because the spec treats bfcached pages and detached iframes very similarly (both are "not fully active"), so if we wanted to distinguish them we'd need to write some tricky spec text. |
There's also the option of Firefox's current policy: we let pages with a BroadcastChannel go into bfcache and the BroadcastChannel remains open, but we remove the page from the bfcache if the page receives a message. This could be generalized to buffering N messages before removing the page from the bfcache. I think there would need to be a reasonably finite N. |
I think I would be fine with either of these options. I believe we implemented the first behavior in WebKit.
|
We had a similar discussion when Chromium supported bfcache for pages with inflight network requests. For network requests, we started with the last option (not caching at all) and then implemented the first option (caching the page with open connections and queueing the messages). With the second option (dropping the messages while in cache), the restored page could be in an unexpected state, so I would prefer the first option (queueing the messages while in cache) in general. |
The concern with queuing is that it leaks information that the page should not have and so e.g. Chrome does not queue sensor updates for pages in BFCache. However broadcast messages are different, the thing broadcasting this info is in your origin and you could choose to queue store and rebroadcast everything when the page comes back out of BFCache, if you wanted, so queuing adds no new leak-capability. That implies that the spec should allow queuing unless I've missed something (and of course allow limiting the queue size). I also agree that dropping could cause unexpected states. I can imagine something that expects that once it starts receiving broadcasts it receives them all with no holes. While we could take a chance on that, it seems unnecessary if queuing is indeed acceptable. |
This CL adds WPTs to test that BroadcastChannels in detached iframes behave according to proposed changes to the specification. For more details, see: whatwg/html#7219 Bug: 1239274 Change-Id: Ia014e412a2dc8267aac57041672835fb90994d89
Isn't queuing problematic for sites using BroadcastChannel for time sensitive state updates? When the page comes out of bfcache suddenly it would get a flurry of messages for old state changes. (These messages would be delivered out of currently spec'd order as well.) These out of order messages could break the site's expectations and lead to unexpected state. (To clarify, I say out-of-order because they are supposed to be delivered to oldest BC first. But if an older BC is in bfcache, then its delivery will be delayed until after other newer BC instances receive the message.) Personally I still like firefox's current policy. Allow pages with BroadcastChannel in the bfcache, but evict if a message would have been delivered to it. This seems to offer the best of both worlds; you get bfcache optimization for relatively quick forward/back navigations, but we don't break spec'd behavior if messages start getting posted. |
I'm not familiar with the spec but reading this section I find 6 Sort destinations such that all BroadcastChannel objects whose relevant agents are the same are sorted in creation order, oldest first. (This does not define a complete ordering. Within this constraint, user agents may sort the list in any implementation-defined manner.) It seems like the creation order is enforced within an agent but across agents there is no ordering constraint. I might have the wrong meaning for "relevant agent" (there's a broken link to the JS spec) but I'm guessing that all of the agents in a BFCached page are frozen and all restarted when they come out of the cache again. If that's correct then this ordering is not a concern. The flurry of events seems less of a worry for correctness but could still cause performance issues. Evict if there are more than N messages could help there. All that said, Chrome's approach has generally been to try the simple thing and see what fraction of the problem it solves. Evicting if any messages arrives while in BFCache seems problem-free, so we should try that but I'd be reluctant to require that in spec until we find out whether that fixes most cases or not. |
Since firefox's behavior is not observable to the page, does it have to say anything in the spec? It would seem changing the spec would only be necessary if chromium wants to change behavior in an observable way. |
Also, its not completely clear why we need to block a decision for |
As I said above,
|
Yea, I read that. But I still don't feel like it answers my question. I understand they both use "not fully active", but it appears that the "transition from not-fully-active to active" use case only applies to bfcache. AFAICT that use case does not exist for detached iframes. So its unclear to me why tricky spec text would be needed to accommodate moving forward with detached iframes if "transition from not-fully-active to active" is not decided yet. |
The question is, how would we spec the detached iframe case? Any spec we would write would have bfcache implications. |
Why is that if bfcache implementations are currently non-observable with respect to BroadcastChannel? Why can't those spec changes come when bfcache folks decide they must make observable changes to BroadcastChannel? |
I'm not really sure how to answer those questions. I can just assure you I don't know how to spec the changes for the iframe case without mandating behaviors for bfcache, which I am not comfortable doing without agreement. |
I spoke with @domenic offline and I think I was a bit confused about one point. I didn't realize the safari behavior "closing" the channel caused BC.postMessage() to throw. That does not seem desirable. I can also see how setting the closed flag on the BroadcastChannel would be difficult for the bfcache case in the future. So I think what I would like to get consensus on is making BroadcastChannel.postMessage() in a detached iframe either drop or queue the message. For the detached iframe case I think these are equivalent in terms of observability. The detached iframe can never become attached again, so even if queued them the queue would never drain. And on the receiving side the spec currently says not to deliver messages at all. That is also observably the same as queuing for the detached iframe case should we want to make that change in the future. |
Dropping makes sense to me for a detached iframe. This also seems like what we could do after self.close() is called in a Worker! |
If we all agreed that FF"s behaviour was the best we could get and that any messages sent while in the cache must cause eviction (e.g. because there was a fatal correctness issue with doing otherwise), then that seems reasonable to spec and to test. I don't think that's currently the case though. |
FYI https://crrev.com/c/3201012 would adds some tests for BFCache and BroadcastChannel. |
@fergald I'm having trouble parsing this. What's the proposal you think makes sense as a path forward for standardization? |
This CL adds tentative WPTs that test whether BroadcastChannels in detached iframes and closed workers behave according to proposed changes to the specification. For more details, see: whatwg/html#7219 Bug: 1239274 Change-Id: Ia014e412a2dc8267aac57041672835fb90994d89
Currently the spec says messages should not be delivered to contexts that are not active. See step 4 at: https://html.spec.whatwg.org/#dom-broadcastchannel-postmessage I think that is observably consistent with evicting bfcache entries when a message would have gone to an inactive context. Maybe there is other bfcache-specific spec text somewhere about when things should be in bfcache or not that would have to change? |
FYI, we have some tentative WPT tests almost ready to land for the detached iframe behavior we have been discussing here: https://chromium-review.googlesource.com/c/chromium/src/+/3229181 Let me know if there is any objection to landing that. Edit: It also includes closing worker tests. |
This CL adds tentative WPTs that test whether BroadcastChannels in detached iframes and closed workers behave according to proposed changes to the specification. For more details, see: whatwg/html#7219 Bug: 1239274 Change-Id: Ia014e412a2dc8267aac57041672835fb90994d89
This CL adds tentative WPTs that test whether BroadcastChannels in detached iframes and closed workers behave according to proposed changes to the specification. For more details, see: whatwg/html#7219 Bug: 1239274 Change-Id: Ia014e412a2dc8267aac57041672835fb90994d89
This CL adds tentative WPTs that test whether BroadcastChannels in detached iframes and closed workers behave according to proposed changes to the specification. For more details, see: whatwg/html#7219 Bug: 1239274 Change-Id: Ia014e412a2dc8267aac57041672835fb90994d89
This CL adds tentative WPTs that test whether BroadcastChannels in detached iframes and closed workers behave according to proposed changes to the specification. For more details, see: whatwg/html#7219 Bug: 1239274 Change-Id: Ia014e412a2dc8267aac57041672835fb90994d89
This CL adds tentative WPTs that test whether BroadcastChannels in detached iframes and closed workers behave according to proposed changes to the specification. For more details, see: whatwg/html#7219 Bug: 1239274 Change-Id: Ia014e412a2dc8267aac57041672835fb90994d89
This CL adds tentative WPTs that test whether BroadcastChannels in detached iframes and closed workers behave according to proposed changes to the specification. For more details, see: whatwg/html#7219 Note that the WPT CI found the existing closing/re-opening test (and two of the new tests) to be flaky in Chrome. From investigating further it seems there's a race condition when instantiating and sending BroadcastChannel messages between a worker and its parent such that using postMessage from either side to indicate BroadcastChannel readiness is insufficient. This CL also adds extra logic to mitigate this in the existing test case. For more info, see: whatwg/html#7267 Bug: 1239274 Change-Id: Ia014e412a2dc8267aac57041672835fb90994d89
This CL adds tentative WPTs that test whether BroadcastChannels in detached iframes and closed workers behave according to proposed changes to the specification. For more details, see: whatwg/html#7219 Note that the WPT CI found the existing closing/re-opening test (and two of the new tests) to be flaky in Chrome. From investigating further it seems there's a race condition when instantiating and sending BroadcastChannel messages between a worker and its parent such that using postMessage from either side to indicate BroadcastChannel readiness is insufficient. This CL also adds extra logic to mitigate this in the existing test case (and the new ones). For more info, see: whatwg/html#7267 Bug: 1239274 Change-Id: Ia014e412a2dc8267aac57041672835fb90994d89
This CL adds tentative WPTs that test whether BroadcastChannels in detached iframes and closed workers behave according to proposed changes to the specification. For more details, see: whatwg/html#7219 Note that the WPT CI found the existing closing/re-opening test (and two of the new tests) to be flaky in Chrome. From investigating further it seems there's a race condition when instantiating and sending BroadcastChannel messages between a worker and its parent such that using postMessage from either side to indicate BroadcastChannel readiness is insufficient. This CL also adds extra logic to mitigate this in the existing test case (and the new ones). For more info, see: whatwg/html#7267 Bug: 1239274 Change-Id: Ia014e412a2dc8267aac57041672835fb90994d89 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3229181 Commit-Queue: Andrew Williams <[email protected]> Reviewed-by: Ben Kelly <[email protected]> Cr-Commit-Position: refs/heads/main@{#935672}
This CL adds tentative WPTs that test whether BroadcastChannels in detached iframes and closed workers behave according to proposed changes to the specification. For more details, see: whatwg/html#7219 Note that the WPT CI found the existing closing/re-opening test (and two of the new tests) to be flaky in Chrome. From investigating further it seems there's a race condition when instantiating and sending BroadcastChannel messages between a worker and its parent such that using postMessage from either side to indicate BroadcastChannel readiness is insufficient. This CL also adds extra logic to mitigate this in the existing test case (and the new ones). For more info, see: whatwg/html#7267 Bug: 1239274 Change-Id: Ia014e412a2dc8267aac57041672835fb90994d89 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3229181 Commit-Queue: Andrew Williams <[email protected]> Reviewed-by: Ben Kelly <[email protected]> Cr-Commit-Position: refs/heads/main@{#935672}
This CL adds tentative WPTs that test whether BroadcastChannels in detached iframes and closed workers behave according to proposed changes to the specification. For more details, see: whatwg/html#7219 Note that the WPT CI found the existing closing/re-opening test (and two of the new tests) to be flaky in Chrome. From investigating further it seems there's a race condition when instantiating and sending BroadcastChannel messages between a worker and its parent such that using postMessage from either side to indicate BroadcastChannel readiness is insufficient. This CL also adds extra logic to mitigate this in the existing test case (and the new ones). For more info, see: whatwg/html#7267 Bug: 1239274 Change-Id: Ia014e412a2dc8267aac57041672835fb90994d89 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3229181 Commit-Queue: Andrew Williams <[email protected]> Reviewed-by: Ben Kelly <[email protected]> Cr-Commit-Position: refs/heads/main@{#935672}
…iframes, closed workers, a=testonly Automatic update from web-platform-tests BroadcastChannel: Add WPTs for detached iframes, closed workers This CL adds tentative WPTs that test whether BroadcastChannels in detached iframes and closed workers behave according to proposed changes to the specification. For more details, see: whatwg/html#7219 Note that the WPT CI found the existing closing/re-opening test (and two of the new tests) to be flaky in Chrome. From investigating further it seems there's a race condition when instantiating and sending BroadcastChannel messages between a worker and its parent such that using postMessage from either side to indicate BroadcastChannel readiness is insufficient. This CL also adds extra logic to mitigate this in the existing test case (and the new ones). For more info, see: whatwg/html#7267 Bug: 1239274 Change-Id: Ia014e412a2dc8267aac57041672835fb90994d89 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3229181 Commit-Queue: Andrew Williams <[email protected]> Reviewed-by: Ben Kelly <[email protected]> Cr-Commit-Position: refs/heads/main@{#935672} -- wpt-commits: de06181503dc24462beb8b1e5cf750ce382c27cf wpt-pr: 31290
…iframes, closed workers, a=testonly Automatic update from web-platform-tests BroadcastChannel: Add WPTs for detached iframes, closed workers This CL adds tentative WPTs that test whether BroadcastChannels in detached iframes and closed workers behave according to proposed changes to the specification. For more details, see: whatwg/html#7219 Note that the WPT CI found the existing closing/re-opening test (and two of the new tests) to be flaky in Chrome. From investigating further it seems there's a race condition when instantiating and sending BroadcastChannel messages between a worker and its parent such that using postMessage from either side to indicate BroadcastChannel readiness is insufficient. This CL also adds extra logic to mitigate this in the existing test case (and the new ones). For more info, see: whatwg/html#7267 Bug: 1239274 Change-Id: Ia014e412a2dc8267aac57041672835fb90994d89 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3229181 Commit-Queue: Andrew Williams <[email protected]> Reviewed-by: Ben Kelly <[email protected]> Cr-Commit-Position: refs/heads/main@{#935672} -- wpt-commits: de06181503dc24462beb8b1e5cf750ce382c27cf wpt-pr: 31290
This CL adds tentative WPTs that test whether BroadcastChannels in detached iframes and closed workers behave according to proposed changes to the specification. For more details, see: whatwg/html#7219 Note that the WPT CI found the existing closing/re-opening test (and two of the new tests) to be flaky in Chrome. From investigating further it seems there's a race condition when instantiating and sending BroadcastChannel messages between a worker and its parent such that using postMessage from either side to indicate BroadcastChannel readiness is insufficient. This CL also adds extra logic to mitigate this in the existing test case (and the new ones). For more info, see: whatwg/html#7267 Bug: 1239274 Change-Id: Ia014e412a2dc8267aac57041672835fb90994d89 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3229181 Commit-Queue: Andrew Williams <[email protected]> Reviewed-by: Ben Kelly <[email protected]> Cr-Commit-Position: refs/heads/main@{#935672}
For BroadcastChannel instances associated with detached iframes or closing workers, update the specification to require that calls to postMessage() should be ignored (without throwing an exception). Closes whatwg#7219. Some potential followup in whatwg#7253 for the related bfcache case.
When there are open BroadcastChannels at the time of navigation, Chrome: The page is not BFCached. The page is BFCached if BroadcastChannels are closed in the pagehide event. Firefox: The page is BFCached. Safari: BroadcastChannel is not supported. Bug: 1107415, whatwg/html#7219 Change-Id: I25afa41274278b0976ad1fa0fdd50015a3f6ce77
When there are open BroadcastChannels at the time of navigation, Chrome: The page is not BFCached. The page is BFCached if BroadcastChannels are closed in the pagehide event. Firefox: The page is BFCached. Safari: BroadcastChannel is not supported. Bug: 1107415, whatwg/html#7219 Change-Id: I25afa41274278b0976ad1fa0fdd50015a3f6ce77 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3201012 Reviewed-by: Fergal Daly <[email protected]> Reviewed-by: Yuzu Saijo <[email protected]> Commit-Queue: Hiroshige Hayashizaki <[email protected]> Cr-Commit-Position: refs/heads/main@{#956437}
When there are open BroadcastChannels at the time of navigation, Chrome: The page is not BFCached. The page is BFCached if BroadcastChannels are closed in the pagehide event. Firefox: The page is BFCached. Safari: BroadcastChannel is not supported. Bug: 1107415, whatwg/html#7219 Change-Id: I25afa41274278b0976ad1fa0fdd50015a3f6ce77 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3201012 Reviewed-by: Fergal Daly <[email protected]> Reviewed-by: Yuzu Saijo <[email protected]> Commit-Queue: Hiroshige Hayashizaki <[email protected]> Cr-Commit-Position: refs/heads/main@{#956437}
When there are open BroadcastChannels at the time of navigation, Chrome: The page is not BFCached. The page is BFCached if BroadcastChannels are closed in the pagehide event. Firefox: The page is BFCached. Safari: BroadcastChannel is not supported. Bug: 1107415, whatwg/html#7219 Change-Id: I25afa41274278b0976ad1fa0fdd50015a3f6ce77 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3201012 Reviewed-by: Fergal Daly <[email protected]> Reviewed-by: Yuzu Saijo <[email protected]> Commit-Queue: Hiroshige Hayashizaki <[email protected]> Cr-Commit-Position: refs/heads/main@{#956437}
…nel, a=testonly Automatic update from web-platform-tests [WPT] BFCache eligibility: BroadcastChannel When there are open BroadcastChannels at the time of navigation, Chrome: The page is not BFCached. The page is BFCached if BroadcastChannels are closed in the pagehide event. Firefox: The page is BFCached. Safari: BroadcastChannel is not supported. Bug: 1107415, whatwg/html#7219 Change-Id: I25afa41274278b0976ad1fa0fdd50015a3f6ce77 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3201012 Reviewed-by: Fergal Daly <[email protected]> Reviewed-by: Yuzu Saijo <[email protected]> Commit-Queue: Hiroshige Hayashizaki <[email protected]> Cr-Commit-Position: refs/heads/main@{#956437} -- wpt-commits: 22074adb41b750abd820301fd731b29d0c9e8793 wpt-pr: 31079
…nel, a=testonly Automatic update from web-platform-tests [WPT] BFCache eligibility: BroadcastChannel When there are open BroadcastChannels at the time of navigation, Chrome: The page is not BFCached. The page is BFCached if BroadcastChannels are closed in the pagehide event. Firefox: The page is BFCached. Safari: BroadcastChannel is not supported. Bug: 1107415, whatwg/html#7219 Change-Id: I25afa41274278b0976ad1fa0fdd50015a3f6ce77 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3201012 Reviewed-by: Fergal Daly <[email protected]> Reviewed-by: Yuzu Saijo <[email protected]> Commit-Queue: Hiroshige Hayashizaki <[email protected]> Cr-Commit-Position: refs/heads/main@{#956437} -- wpt-commits: 22074adb41b750abd820301fd731b29d0c9e8793 wpt-pr: 31079
For BroadcastChannel instances associated with detached iframes or closing workers, update the specification to require that calls to postMessage() should be ignored (without throwing an exception). Closes whatwg#7219. Some potential followup in whatwg#7253 for the related bfcache case.
This CL adds tentative WPTs that test whether BroadcastChannels in detached iframes and closed workers behave according to proposed changes to the specification. For more details, see: whatwg/html#7219 Note that the WPT CI found the existing closing/re-opening test (and two of the new tests) to be flaky in Chrome. From investigating further it seems there's a race condition when instantiating and sending BroadcastChannel messages between a worker and its parent such that using postMessage from either side to indicate BroadcastChannel readiness is insufficient. This CL also adds extra logic to mitigate this in the existing test case (and the new ones). For more info, see: whatwg/html#7267 Bug: 1239274 Change-Id: Ia014e412a2dc8267aac57041672835fb90994d89 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3229181 Commit-Queue: Andrew Williams <[email protected]> Reviewed-by: Ben Kelly <[email protected]> Cr-Commit-Position: refs/heads/main@{#935672} NOKEYCHECK=True GitOrigin-RevId: 0be41ddeb37f733a7594164d753431694d4f53b2
When there are open BroadcastChannels at the time of navigation, Chrome: The page is not BFCached. The page is BFCached if BroadcastChannels are closed in the pagehide event. Firefox: The page is BFCached. Safari: BroadcastChannel is not supported. Bug: 1107415, whatwg/html#7219 Change-Id: I25afa41274278b0976ad1fa0fdd50015a3f6ce77 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3201012 Reviewed-by: Fergal Daly <[email protected]> Reviewed-by: Yuzu Saijo <[email protected]> Commit-Queue: Hiroshige Hayashizaki <[email protected]> Cr-Commit-Position: refs/heads/main@{#956437} NOKEYCHECK=True GitOrigin-RevId: 24d9482b643e5e04fc36fe30a5a06fe37502b1a2
Currently the spec checks for a fully active context when determining which contexts should receive messages. See step 4 here:
https://html.spec.whatwg.org/#dom-broadcastchannel-postmessage
But it does not check for a fully active context when creating or sending a message. So in theory this should work:
This feels a bit weird. Its also causing us implementation problems for our BroadcastChannel partitioning efforts.
Would other browsers be open to causing
new BroadcastChannel()
to throw InvalidStateException? Andbc.postMessage()
to have no effect?Also note, it appears neither firefox or chrome currently prevent delivery of BC messages to detached iframes today. We would like to implement that as well.
@asutherland @bakulf @cdumez
The text was updated successfully, but these errors were encountered: