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

How should BroadcastChannel and BFCache interact? #7253

Open
fergald opened this issue Oct 21, 2021 · 16 comments
Open

How should BroadcastChannel and BFCache interact? #7253

fergald opened this issue Oct 21, 2021 · 16 comments
Labels
interop Implementations are not interoperable with each other topic: history topic: serialize and transfer

Comments

@fergald
Copy link

fergald commented Oct 21, 2021

Forking off from #7252. I don't think this is urgently needed, just trying to gather all the info in one place.

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.

@fergald I'm having trouble parsing this. What's the proposal you think makes sense as a path forward for standardization?

The question for BFCache is whether a message being sent to a BFCached frame should cause the frame/page to become unsalvageable. What's specced right now is that the message should not be delivered.. We can

1 leave it as is
2 spec that it makes it unsalvageable
3 spec that it may be queued and delivered if the page is restored (size of queue is up to implementation, marking as unsalvageable is OK at any point)
4 spec that it's dropped
5 note that it's currently unresolved

1 and 3 are somewhat equivalent in that if we don't disallow it, a browser is not out of spec for doing it (although it's following an unspecced queuing algorithm).

2 is Firefox's behaviour.
3 is nobody's behaviour but does not seem incompatible with current spec and doesn't seem raise security or privacy issues. It could be a surprise to some pages to receive events late or in quick succession as the queue is flushed (arguably no worse than if a page ran a very long task).
4 is nobody's behaviour either but dropping messages violates expectations of reliability

I would just like to avoid speccing or testing that we cannot queue and deliver later, unless someone has a strong reason for it being wrong.

@asutherland @wanderview @rubberyuzu @cdumez @domenic

domenic pushed a commit that referenced this issue Nov 5, 2021
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 #7219. Some potential followup in #7253 for the related bfcache case.
dandclark pushed a commit to dandclark/html that referenced this issue Dec 4, 2021
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.
mfreed7 pushed a commit to mfreed7/html that referenced this issue Jun 3, 2022
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.
@domenic
Copy link
Member

domenic commented Jun 14, 2022

Apparently Firefox's behavior has compat issues with GitHub itself; see https://bugzilla.mozilla.org/show_bug.cgi?id=1772739#c10 . This might be a motivation for different browsers to get on the same page here, to avoid these sorts of compat issues.

@smaug----
Copy link

Note, that Github issue seems to be something very recent, they have changed something on their side, I think.

@smaug----
Copy link

I don't know what unsalvageable means in this context. It is same as evicting from bfcache?
Also, note that Chrome evicts from bfcache when using client.postMessage() and the receiving side is in bfcache. I'd expect some consistency with various postMessage cases.

@fergald
Copy link
Author

fergald commented Jun 15, 2022

"unsalvageable" is the spec language for eviction. An inactive document that is still salvageable can still be navigated to.

BroadcastChannel prevents < 0.1% of BFCache usage, so it's low priority for chrome to improve.

So, the question is whether we should spec that receiving a broadcast message evicts. What is the bad thing that would happen some impl decided to just queue the messages and deliver them on restore?

@domenic
Copy link
Member

domenic commented Jun 15, 2022

"unsalvageable" is the spec language for eviction. An inactive document that is still salvageable can still be navigated to.

This isn't accurate. Salvageability is used for determining whether a document can stay in bfache at unload time. Eviction consists of nulling out a session history entry's document at some later time and is unrelated to salvageability.

@fergald
Copy link
Author

fergald commented Jun 15, 2022

Thanks for the correction. I see

"Apart from these restrictions, this standard does not specify when user agents should discard an entry's document, versus keeping it cached."

This sentence here is odd. I guess we have never specced an eviction case yet but that makes it sound like we won't (as opposed to coincidentally just haven't yet).

@annevk
Copy link
Member

annevk commented Jun 15, 2022

Agreed, we should reword that. Having that defined would also help with whatwg/fs#17 (comment).

@rakina
Copy link
Member

rakina commented Jun 15, 2022

Yeah I don't think anything currently specs eviction that happen after unload steps. In addition to updating the note, I think we should also add a hook to spec eviction that other specs can refer to (that can be called with just the document, so the callers don't need to find the relevant entries that needs to have their document nulled out?)

@fergald
Copy link
Author

fergald commented Jun 16, 2022

So should we just extend the use of salvageable to mean what I thought it meant, i.e. a document can be marked unsalvageable when in BFCache and an unsalveagable document gets it's entries nulled out?

@rakina
Copy link
Member

rakina commented Jun 16, 2022

I think we have two options here:

  1. Also check the "salvageable" bit in this part of history traversal that determines if a document can be reused (if not, we just null out the document)
  2. Provide a hook that will null out the entry immediately

#1 seems more natural but #2 is closer to what Chrome actually does in implementation. I'm not sure if there are implicit side effects of not evicting the entry until we actually traverse to it, but #1 seems preferable to me. I can send a PR if there are no problems with that.

@annevk
Copy link
Member

annevk commented Jun 16, 2022

I think for certain cases we need 2, right? E.g., some other document wants to obtain a lock so we'll just pretend that the bfcache entry with a lock never existed.

@domenic
Copy link
Member

domenic commented Jun 16, 2022

I would prefer (2). "Salvageable" is about the act of "salvaging", which I think is best interpreted as "keeping the document even when you're throwing away a bunch of other stuff".

@fergald
Copy link
Author

fergald commented Jun 17, 2022

I would prefer (2). "Salvageable" is about the act of "salvaging", which I think is best interpreted as "keeping the document even when you're throwing away a bunch of other stuff".

In my mind the salvaging happened when the traversal occurred, i.e. salvage = restore. I guess I got that from

https://html.spec.whatwg.org/multipage/browsing-the-web.html#the-pagetransitionevent-interface

meaning that the page might be reused if the user navigates back to this page (if the Document's salvageable state stays true).

but having read the steps more closely, I agree that a separate nulling operation makes sense.

@fergald
Copy link
Author

fergald commented Jun 17, 2022

So to come back to the original question.

What should the spec say about a page with an open BCs?

  1. that it makes the page unsalvageable (Chrome's today)
  2. that it can go in BFCache but any message on the BC will cause it to be evicted (Mozilla today)
  3. that messages on the BC can be queued and delivered if the page is restored

I think we all agree that it's not 1. but is it necessary to spec that it's 2.? If some browser wanted to implement 3. what would be the correctness or privacy problem? Obviously implementation-wise you wouldn't want to queue lots but 1 or 2 seems harmless. Spec-wise, it looks like 3. would just require removing the "is fully active" qualification from eligible for messaging.

@asutherland
Copy link

The advantage of specifying 2 (evict on receipt of any message) for now:

  • It's arguably easier to relax this constraint than to tighten them, although you could experience breakage in any direction.
  • It's consistent with other (non-broadcast) uses of postMessage where it's more clear that evicting on receipt of message makes sense.
  • We don't have Augment structured serialize algorithm to optionally compute a canonical serialized length to allow cross-browser consistent data limits #4914 / Define size of all storage actions storage#110 yet which means that the only way to specify a consistent cross-browser behavior with buffered messages is based on the number of messages and to ignore the amount of resources associated with the messages. This means that it becomes very easy to accidentally entrain a bunch of resources for messages that included a large ArrayBuffer or a bunch of Blobs/Files or anything else that doesn't explicitly need to be transferred and can be copied.
    • One could argue that browsers will be already be making bfcache eviction decisions based on dynamic resource availability that will vary wildly between workloads and are impossible to standardize, but I think it would be good to understand what benefit we expect from buffering 1 or 2 messages. Like, if I was using BroadcastChannel to send some kind of "heartbeat" message I would be very frustrated to have a page come out of BFCache, immediately receive a "heartbeat" message and then have no idea if that was a timely heartbeat message or an old heartbeat message. Obviously, someone can put Date.now() in there (but not Performance.now() which would potentially be meaningless) after they've been burned.

@fergald
Copy link
Author

fergald commented Jun 20, 2022

Yeah, I don't think we should or could standardize queue sizes (at most we could standardize max queue sizes since browsers are free to evict whenever they want for no user-visible reason).

I also expect that the incremental benefit of queuing some message is probably much lower than that of allowing pages into the cache until a message is sent (I assume that the frequent heartbeats use case is rare). We will know this from our telemetry in Chrome once we get around to allowing this but currently all BC accounts for < 1% of all non-cached history navigations.

So then if we were to spec that messages cause eviction, it would simply be because we think it's better to have predictable behaviour than get that last tiny fraction of caching. That's fine with me, I just want to be explicit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interop Implementations are not interoperable with each other topic: history topic: serialize and transfer
Development

No branches or pull requests

6 participants