Bug 29021 – Explore allowing pages with unload handlers into the Page Cache

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Brady Eidson]

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>

[Sam Weinig]

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.

[Brady Eidson]

Created attachment 39585 [details]
Disable the unload handler check on Mac + Windows

[Brady Eidson]

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.

[Brady Eidson]

Created attachment 39586 [details]
Slightly better preprocessor style this time

[Brady Eidson]

Committed r48380: <http://trac.webkit.org/changeset/48380>

[Antti Koivisto]

I think you mean http://trac.webkit.org/changeset/48388

[Brady Eidson]

Yes, I did, and I'm never using bugzilla-tool again!

[Eric Seidel (no email)]

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

[Brady Eidson]

Re-opening, as this bug needs to track the possibility of putting this check back in to place.

[Brady Eidson]

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.

[Sam Weinig]

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?

[Brady Eidson]

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.

[Brady Eidson]

http://trac.webkit.org/changeset/48456

[Jake Archibald]

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.

[Paul Sowden]

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!

[Timothy Hatcher]

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…

[Eric Seidel (no email)]

I assume this can be closed now that it's landed?

[Brady Eidson]

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.

[Eric Seidel (no email)]

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. :(

[Eric Seidel (no email)]

Comment on attachment 39586 [details]
Slightly better preprocessor style this time

Clearing darin's review on this obsolete patch.

[Eric Seidel (no email)]

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.

[Brady Eidson]

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.

[Brady Eidson]

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.

[Jake Archibald]

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.

[Brady Eidson]

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.

[Grace Kloba]

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.

[Jake Archibald]

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.