292950 - Assert on reload

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE:

1)  Build and enable bfcache
2)  Start browser
3)  Load http://www.mozilla.org/
4)  Wait for load to finish
5)  Click "reload" button

EXPECTED RESULTS: reload quietly

ACTUAL RESULTS:

###!!! ASSERTION: RestorePresentation should only be called for history loads:
'mLoadType & LOAD_CMD_HISTORY', file
/home/bzbarsky/mozilla/vanilla/mozilla/docshell/base/nsDocShell.cpp, line 4952

Comment 1

[Boris Zbarsky [:bzbarsky]]

OK, this is actually happening with bfcache disabled too.  Could we get some
traction on this?  Why are we ending up inside RestorePresentation() for
non-history loads, exactly?

Comment 2

[Boris Zbarsky [:bzbarsky]]

To be precise, this is happening for loads with mType == LOAD_RELOAD_NORMAL. 
Those have a SHEntry, but are not exactly history loads in the sense we want
here.  That is, I don't think we should be doing fastback-like stuff for
LOAD_RELOAD_NORMAL.

We should probably just change this assert to a check-and-early-return, even
before we look at the history entry.

Comment 3

[chris hofmann]

be good if we could resolve in firefox 1.1.  see
https://bugzilla.mozilla.org/show_bug.cgi?id=295813

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: patch to fix

Don't restore saved presentation unless there is one

Comment 5

[Boris Zbarsky [:bzbarsky]]

Doesn't this skip saving the current presentation too?  That looks wrong...

Comment 6

[Boris Zbarsky [:bzbarsky]]

Er, nevermind comment 5.

Comment 7

[Brian Ryner (not reading)]

Comment on attachment 185894 [details] [diff] [review]
patch to fix

Seems better to just check at the call site whether (mLoadType &
LOAD_CMD_HISTORY)?  That's the assumption of RestorePresentation, not that the
viewer is non-null.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The reason i checked the viewer was that otherwise we'd end up calling
SyncPresentationState on both SHEntries. That seemed like unneccesary work?

Also, it seemed like a good sanity check to assert if we have a viewer but
aren't performing a history load. Though that could be added separatly.

Comment 9

[Brian Ryner (not reading)]

Maybe we could only do the SyncPresentationState work for the case where
NS_FAILED(rv)?  The only cases where |restored| is false, but the method
succeeds are:

1. no saved presentation
2. the saved content viewer has a different container

In the first case here, there's no need to SyncPresentation, but in the second
case, since we null out the saved content viewer, we probably do want to
SyncPresentation.  So maybe we do this:

1. Change case #2 above to return a failure code
2. Make the SyncPresentation work only happen if NS_FAILED(rv).
3. Wrap the whole thing with a check of mLoadType

Comment 10

[Brian Ryner (not reading)]

fixed by bug 292954 checkin