305236 - The bfcache stores too many things in the cache

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

Currently, when we have a window.open(url) call, we first load an about:blank
page and then load the actual page in that same docshell/window. While the
about:blank page doesn't make it into session history (i.e., you can't click
back to go to it), the bfcache *does* store the about:blank page in the cache.
This confuses us, since this is a case where we want to reuse the (about:blank)
inner window.

The solution is to not put the about:blank document in the bfcache.wodniw ren

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

With this fix, we refuse to try to store the current page in the bfcache if
we're going to reuse the inner window. Note that in these cases, mOSHE is never
actually put into the session history, so storing the content viewer really is
wrong.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 193227 [details] [diff] [review]
Fix

jst and I talked about this and WouldReuseInnerWindow is going to take a
document instead of a URI.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: pass the document instead of the URI

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 193234 [details] [diff] [review]
pass the document instead of the URI

I'm sorry, I'm rushing too much. aRemoveEventListeners doesn't seem to get set
correctly with this patch. I propose to do: aRemoveEventListeners =
!reUseInnerWindow; though we should probably get rid of that parameter
altogether.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: set aRemoveEventHandlers too

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Nominating for blocking, since this blocks a blocking 1.8b4+ bug.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 193236 [details] [diff] [review]
set aRemoveEventHandlers too

sr=jst

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk. I'll check this into the branch tomorrow.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 193236 [details] [diff] [review]
set aRemoveEventHandlers too

This needs to be checked in on the branch.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Checked into MOZILLA_1_8_BRANCH.