We should explore allowing pages with unload handlers into the Page Cache. Historically, we decided to discount these pages under the assumption that unload handlers do critical work. Firefox seemed to follow suite a few years later, as evidenced by https://developer.mozilla.org/En/Using_Firefox_1.5_caching In recent months, while exploring enhancements we can make to the page cache, it's become clear that *many* pages are discounted from our page cache because of unload handlers. And - quite frankly - most of them don't do important work, don't do interesting work, or don't even do any work at all. We've now added the pagehide/pageshow events that Firefox added to help developers work around this page caching limitation. Also, the landscape of the web has changed quite a bit since 2003. We should explore whether or not keeping this limitation is worthwhile. This exploration might be as simple as checking in the one line change that will allow these pages in to the page cache, and watch and see if anything important breaks. In radar as <rdar://problem/7196485>
I would prefer if this went behind an ENABLE #define to allow ports to easily choose their behavior while this is still under investigation. In general, this seems like it could impose a functional regression and one with subtle and hard to diagnose fallout at that, and thus giving flexibility during the exploration seems prudent.
Created attachment 39585 [details] Disable the unload handler check on Mac + Windows
Once this gets checked in, I have a blog post prepared that will discuss the state of the page cache, and put a call out to site/library developers to experiment and comment.
Created attachment 39586 [details] Slightly better preprocessor style this time
Committed r48380: <http://trac.webkit.org/changeset/48380>
I think you mean http://trac.webkit.org/changeset/48388
Yes, I did, and I'm never using bugzilla-tool again!
(In reply to comment #7) > Yes, I did, and I'm never using bugzilla-tool again! Hum. I've never seen it post the wrong revision number like that, but I would love to find out why! I'll follow up with you via email and file/fix any necessary bugs.
Re-opening, as this bug needs to track the possibility of putting this check back in to place.
Created attachment 39681 [details] Don't fire unload events when leaving a page if the document has been cached. As long as we're running this experiment, lets run it right! Don't fire unload events if the document was put in the Page Cache.
Comment on attachment 39681 [details] Don't fire unload events when leaving a page if the document has been cached. You sure this can't be tested?
I can't think of a good way right now that wouldn't some *very* ugly hacking in WebCore simply for DRTs benefit. Perhaps it's my lack of caffiene and sleep. If I think of one in the morning, I'll add it.
http://trac.webkit.org/changeset/48456
You could save the page to the page cache, then fire the unload event and navigate away. If back is clicked, the page will be returned in the state it was BEFORE unload was fired. This gives you the best of both worlds. (this was suggested in Chrome's bug tracker) If you're going to fire unload when the page is removed from memory (rather than when it is navigated away from) you need to be careful with "Your work is unsaved, are you sure you want to navigate away?" type messages. Applications like Gmail use these. However, the solution above works around this. Also, (and webkit may do this already) you need to make sure the element that the mouse is currently over has its mouseout event fired. Else you might get sticky hover effects. It's great that this is being looked into, as many js libraries use the unload event to clean up in IE & prevent the sticky hover issue, breaking the page cache. Jake.
Hey, I just read the blog post over on Sufin' Safari (really good btw) and I thought I'd throw out our use case for the onunload event as a data point. I work at Meebo and we have two pieces of JavaScript that make heavy use of this event, meebo.com and a Meebo chat bar that is syndicated on partner Web sites. In both of these cases we're using long polling to maintain a connection to our servers to power IM presence for the user. We use the onunload event to send a "quit" command to the server to log out of IM when the page is navigated away. For meebo.com (which is a destination IM Web site) we also use the onbeforeunload event to warn the user that navigating from the page will cause their session to be disconnected. When the user navigates away from the page we rely on the onunload event in order to disconnect the user from their session, and I actually think in this case it would be undesirable for the quick "Page Cache" backwards navigation, as this would imply that we would reconnect you to IM, when I feel like navigating away from this page should probably tear down the page and land you back on a blank slate if you were to navigate back. Not firing the onunload event is also undesirable as you'll appear to stay online for another minute or two after you've left before our server times out the session. For the Meebo chat bar (which is conceptually very similar to Facebook's chat bar) your presence should actually follow you based on whether you're on the page or not so in this case implementing the pageshow/pagehide events rather than the onunload event would be ideal, and I hope that we'll be able to do this in the not too distant future! I hope these use cases are useful!
On the flipside, meebo.com could use pagehide to disconnect and pageshow to reconnect you. This would let you keep the same IM "windows" open, and all other sate. That is what I would expect as a meebo.com user. But I guess there are security concerns…
I assume this can be closed now that it's landed?
No. Unfortunately bugzilla doesn't have a great notion of "temporarily change the code in a given manner and track an experiment", but that's what we're doing here.
Sounds good. I'm going to clear the r+ flags since our "to-be-committed" queue doesn't understand "temporary change in code to try an experiment" either. :(
Comment on attachment 39586 [details] Slightly better preprocessor style this time Clearing darin's review on this obsolete patch.
Comment on attachment 39681 [details] Don't fire unload events when leaving a page if the document has been cached. Clearing sam's review now that this patch has been landed.
We haven't seen a rash of bug reports in the WebKit project as we expected we might. But we do know: -Disabling this exemption cause a couple of benchmark performance regressions -Some important corporate intranet apps rely on the status-quo unload handler behavior, and this change breaks them. -Other browsers have seen substantial negative effects in analytics/marketshare/usage numbers when they fired unload handlers less often than the status quo. While the perf. regressions could be explored and fixed, then other two points are pretty damning. For now I think we're re-enabling this check.
Re-enabled in http://trac.webkit.org/projects/webkit/changeset/50153 I'll close this bug for now. If/when we come up with new ideas to improve this situation in the future, I'll file a new bug.
What about my earlier comment about caching the state of the page before unload? If it's being ignored because the comment should have been made elsewhere, let me know. "We haven't seen a rash of bug reports in the WebKit project as we expected we might." I'm very surprised by this. The latest version of jQuery (1.3.2) detaches ALL event listeners on unload. This should have broken most interactivity on the page added via jQuery. I've just checked in the jQuery trunk, they've changed this code so it only runs in IE, however that isn't in an official jQuery release yet. "Other browsers have seen substantial negative effects in analytics/marketshare/usage numbers when they fired unload handlers less often than the status quo." Is this really a deal breaker? Shouldn't the priority be the user experience rather than usage stats? Could you work around this by sending an async request to the page after it's called back from the page cache? "Some important corporate intranet apps rely on the status-quo unload handler behavior, and this change breaks them." Do these also operate in standards mode? Either way, caching the state of the page before unload is called should solve this. Jake.
I thought I replied to your suggestion when you originally made it. Sometimes I fail at using bugzilla correctly ;) "You could save the page to the page cache, then fire the unload event and navigate away. If back is clicked, the page will be returned in the state it was BEFORE unload was fired." There's two major problems with this approach. 1 - It's technologically VERY very difficult. No, not impossible, and we're not afraid of doing difficult things, but this would require a major re-architecturing of both JavaScripteCore and WebCore to accomplish. A major undertaking that we'd need to conclusively know was worth it. 2 - Loads and unloads need to be balance. What you're suggesting opens the possibility that unload is fired twice (Or more!) for only a single corresponding load event. This is bad(tm) :) >Is this really a deal breaker? Shouldn't the priority be the user experience rather than usage stats? It's not only usage stats - it's ALL negative side effects from us not running code that programmers think we'll run. This whole experiment was about pushing the limits in balancing user experience and fulfilling an API contract with developers. We learned a lot. We raised awareness. This is good for users in the long run. >Could you work around this by sending an async request to the page after it's called back from the page cache? I encourage you to follow progress in https://bugs.webkit.org/show_bug.cgi?id=30457 and https://bugs.webkit.org/show_bug.cgi?id=30458. But without some heuristic manner of determining all of the side effects of running an unload handler (which we most certainly do not have) that still doesn't solve the page cache problem.
Brady, what happen if we call unload as before, which means reverting http://trac.webkit.org/changeset/48456, then we call m_document->dispatchWindowLoadEvent() in CachedFrameBase::restore(). This way, loadEvent and unloadEvent are balanced. Even unload may destroy some critical part, it is better than not cache at all.
It'd be interesting to test, but I predict it would break pages. It's very rare for an unload handler to be the opposite of a load handler. Load tends to be used to modify elements on the page & start timers, unless people are reverting these things in unload, you'll start getting duplicates appearing on the page.