292954 - bfcache/fastback doesn't play nicely with existing loads

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 274784, the "resuming network activity on back" testcase.  Basically,
loads that are in-progress when we leave a page are stopped and not restarted on
fastback.

Comment 1

[Frank Wein [:mcsmurf]]

*** Bug 293752 has been marked as a duplicate of this bug. ***

Comment 2

[Brian Ryner (not reading)]

Ok, it seems that the reason that the testcase fails is that by the time we do
the _second_ CanSavePresentation check (the one that happens from
CreateContentViewer), the request in the iframe document's load group has
already been removed.

This seems like a pretty good argument for finding a way to only do the
CanSavePresentation check once, in InternalLoad(), storing the result off
somewhere.  Once we've called Stop(), it's probably too late.

Comment 3

[Brian Ryner (not reading)]

Attachment: patch

Only call CanSavePresentation once, before we call Stop().  Use
mSavingOldViewer to retain the result.

I also discovered that when loading the iframe, mOSHE == mLSHE for the initial
load (with both of them pointing to the SHEntry for the newly-loading
document).  I'm not sure whether this is correct or not, but in any case, we
probably shouldn't try to save state in this case since we'll be saving state
into a session history entry which we're still going to be displaying.

Comment 4

[Boris Zbarsky [:bzbarsky]]

I won't be able to do a thorough review until I get back in July (have to catch
a plane within the next 12 hours, and no email till I get back after that), but
that patch looks wrong -- a load that starts after the initial InternalLoad but
before we actually load the new document will be killed off by the Stop() we do
when the new data starts coming in and not restarted on back.  I also suspect
the patch doesn't handle cases when InternalLoad is called before the previous
load "completes" (or even returns from server) well.

Comment 5

[Brian Ryner (not reading)]

Ok, so perhaps we save the presentation only if there were no pending requests 
in InternalLoad and there are also no pending requests when the new data 
arrives.  That's easy enough.  What's important is that we not lose the fact 
that there were pending requests before the first Stop().

Note: the entire extent to which I'm planning to resolve this right now is to 
avoid caching the presentation if there is anything to be loaded, restarting 
things is too difficult given the time constraints.

I'll check out that approach and have Darin review, I think he has a good 
handle on DocShell loading.

Comment 6

[Brian Ryner (not reading)]

Attachment: patch

Check for pending loads again when new data arrives, but only go ahead with
saving the presentation if there were also no pending loads before calling
Stop().

Comment 7

[Brian Ryner (not reading)]

Attachment: patch

Ok, this actually addresses a couple of additional issues:

1. fixed the reload on assert (bug 292950)
2. fixed a problem where we were examining the previous load type instead of
the current load type when determining whether to cache.  this leads to the
page not being cached if you had just reloaded it before leaving.

This is all rolled into one patch because it all touches the same methods and I
don't want to spend time untangling it.

Comment 8

[Darin Fisher]

Comment on attachment 186974 [details] [diff] [review]
patch

r+sr=darin

Comment 9

[Brian Ryner (not reading)]

Comment on attachment 186974 [details] [diff] [review]
patch

requesting approval, only affects fastback

Comment 10

[Brian Ryner (not reading)]

checked in

Comment 11

[Boris Zbarsky [:bzbarsky]]

Brian, thanks for fixing this!  Verified on the testcase in a current trunk build.