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

salvagable when unloading a document with pagehide listener #3729

Open
zcorpan opened this issue Jun 1, 2018 · 10 comments
Open

salvagable when unloading a document with pagehide listener #3729

zcorpan opened this issue Jun 1, 2018 · 10 comments

Comments

@zcorpan
Copy link
Member

zcorpan commented Jun 1, 2018

See web-platform-tests/wpt#11294

I have not investigated much about what the browser behavior really is, but for this test it seems salvagable should be false (or at least the setTimeout doesn't survive the navigation in browsers).

@annevk
Copy link
Member

annevk commented Jun 1, 2018

Nothing in https://searchfox.org/mozilla-central/source/dom/base/nsDocument.cpp#7997 seems to support this though. So there must be a different reason.

@zcorpan
Copy link
Member Author

zcorpan commented Jun 1, 2018

@bzbarsky any clue?

@bzbarsky
Copy link
Contributor

bzbarsky commented Jun 1, 2018

I would really appreciate a clearer description of what you're asking. Which of the tests web-platform-tests/wpt#11294 touches are we talking about here? Which of the multiple setTimeout calls in those tests are we talking about?

@bzbarsky
Copy link
Contributor

bzbarsky commented Jun 1, 2018

In particular, if we're talking about html/browsers/browsing-the-web/unloading-documents/unload/manual-001.html then I do see "PASS" when I go back in that test, in Firefox.

So are you talking about html/browsers/browsing-the-web/unloading-documents/unload/006-1.html then?

@zcorpan
Copy link
Member Author

zcorpan commented Jun 1, 2018

Sorry, html/browsers/browsing-the-web/unloading-documents/unload/006-1.html , this timeout

   setTimeout(function() {
-               parent.t.done()
+               parent.t.step_func(function() { assert_unreached('setTimeout survived navigatoin'); })
              }, 1000);

@bzbarsky
Copy link
Contributor

bzbarsky commented Jun 1, 2018

Ah, thank you for explaining.

In Firefox only toplevel navigations get bfcached. So if navigation is happening in a subframe, as here, the document will never be considered salvagable. This seems to be a violation of the spec, because as far as I can tell the spec requires bfcaching in all cases(!). That seems like a spec bug. Bfcaching is not a feature that should be required.

@zcorpan
Copy link
Member Author

zcorpan commented Jun 1, 2018

Ok, turning off bfcache for non-top-level BCs seems reasonably simple to fix in the spec.

@bzbarsky
Copy link
Contributor

bzbarsky commented Jun 1, 2018

I think the spec should allow, but not require, bfcache. Period. Browsers should be free to not bfcache for whatever reason they want to...

@zcorpan
Copy link
Member Author

zcorpan commented Jun 1, 2018

Sure, it can do that also. But if all browsers agree to disable it for iframes, then the spec can codify that, and have bfcache be optional just for top-level.

@bzbarsky
Copy link
Contributor

bzbarsky commented Jun 1, 2018

I guess. The only reason Firefox didn't enable it for iframes was that it was more work that we haven't found the time/desire to do yet.... There's no conceptual reason to forbid it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants