html icon indicating copy to clipboard operation
html copied to clipboard

How should BroadcastChannel and BFCache interact?

Open fergald opened this issue 4 years ago • 16 comments

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

fergald avatar Oct 21 '21 01:10 fergald

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.

domenic avatar Jun 14 '22 13:06 domenic

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

smaug---- avatar Jun 14 '22 13:06 smaug----

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.

smaug---- avatar Jun 14 '22 14:06 smaug----

"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?

fergald avatar Jun 15 '22 00:06 fergald

"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.

domenic avatar Jun 15 '22 02:06 domenic

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).

fergald avatar Jun 15 '22 02:06 fergald

Agreed, we should reword that. Having that defined would also help with https://github.com/whatwg/fs/issues/17#issuecomment-1150080182.

annevk avatar Jun 15 '22 08:06 annevk

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?)

rakina avatar Jun 15 '22 14:06 rakina

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?

fergald avatar Jun 16 '22 02:06 fergald

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.

rakina avatar Jun 16 '22 03:06 rakina

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.

annevk avatar Jun 16 '22 09:06 annevk

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".

domenic avatar Jun 16 '22 12:06 domenic

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 avatar Jun 17 '22 03:06 fergald

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.

fergald avatar Jun 17 '22 08:06 fergald

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 https://github.com/whatwg/html/issues/4914 / https://github.com/whatwg/storage/issues/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.

asutherland avatar Jun 17 '22 17:06 asutherland

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.

fergald avatar Jun 20 '22 04:06 fergald