Do not prevent bfcache if all BroadcastChannel has been closed

RESOLVED FIXED in Firefox 44

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: vnicolas, Assigned: baku)

Tracking

unspecified
mozilla44
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
It seems like BFCache is disabled if BroadcastChannel is used. It kind of makes sense if the channel is open, but I don't see a good reason to prevent BFCache if all BroadcastChannel instances of a page are closed.

Andrea, is there any spec reasons to disallow BFCache in this case, or was it just written this way and it can be changed ?
Flags: needinfo?(amarchesini)
bfcache isn't really a spec thing. UA may or may not have such, and it is up to the UA to decide when it is used (and when pages in bfcache are evicted).
So, sounds ok to me to not prevent bfcache if all the BChannels the page has used are closed.
(Assignee)

Updated

2 years ago
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
(Assignee)

Comment 2

2 years ago
Created attachment 8671478 [details] [diff] [review]
bf2.patch
Attachment #8671478 - Flags: review?(bugs)
Comment on attachment 8671478 [details] [diff] [review]
bf2.patch

>+++ b/dom/broadcastchannel/BroadcastChannelChild.cpp
>@@ -115,16 +115,18 @@ BroadcastChannelChild::RecvNotify(const 
>     return true;
>   }
> 
>   event->SetTrusted(true);
> 
>   bool status;
>   mBC->DispatchEvent(static_cast<Event*>(event.get()), &status);
> 
>+  mBC->RemoveDocFromBFCache();
>+
I think you want the call here before dispatching the event, probably right before
if (NS_FAILED(mBC->CheckInnerWindowCorrectness())) {


You'll add some tests, right?
Attachment #8671478 - Flags: review?(bugs) → review+
(Assignee)

Comment 4

2 years ago
Created attachment 8671522 [details] [diff] [review]
test

This test is not enough because doc->GetBFCacheEntry() returns null.
I need a feedback. Thanks!
Attachment #8671522 - Flags: feedback?(bugs)
[21:59]	smaug	Because of RemoveFromBFCacheSync(), we should test that a page is moved to bfcache (pagehide.persisted == true) but when taken out from there, (history.back()) pageshow.persisted == false
[21:59]	smaug	_if_ bchannel is used while the page is in bfcache
(Assignee)

Comment 6

2 years ago
Created attachment 8672674 [details] [diff] [review]
test
Attachment #8671522 - Attachment is obsolete: true
Attachment #8671522 - Flags: feedback?(bugs)
Attachment #8672674 - Flags: review?(bugs)
Comment on attachment 8672674 [details] [diff] [review]
test

>+    if (counter == 0) {
>+      ok(!e.persisted, "test page should have been persisted initialy");
initially


Hard to follow test.
Attachment #8672674 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/295423abf942
https://hg.mozilla.org/mozilla-central/rev/3ac968ebc057
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.