68847 - go to framed window, reload breaks back button

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect, P3)

Description

[Christian Reis]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux 2.4.1 i586; en-US; 0.8) Gecko/20010214
BuildID:    2001021403

Blocking image on a framed page and reloading breaks back button with a
JavaScript error: 
 line 0: uncaught exception: [Exception... "Component returned failure code:
0x80004005 (NS_ERROR_FAILURE) [nsIWebNavigation.goBack]"  nsresult: "0x80004005
(NS_ERROR_FAILURE)"  location: "JS frame ::
chrome://navigator/content/navigator.js :: BrowserBack :: line 572"  data: no]

error!


Reproducible: Always
Steps to Reproduce:
Load up www.pro.com. Block the image on the top right of page. Reload.

Actual Results:  Back button breaks.

Expected Results:  Back button should work.

Comment 1

[Asa Dotzler [:asa]]

setting bug status to New.

Comment 2

[Paul Chen]

Mass moving most of mozilla0.9 bugs to mozilla0.8.1

Comment 3

[Stephen P. Morse]

Changing OS from linux to all because I'm seeing this behavior on win32 also.

This has nothing to do with image blocking -- that was just a red herring.  Try 
the identical test but omit the block-image step and again the back button will 
be broken.  Removing the reference to block-image in the summary.

Also changing the component away from cookies (cookies is the defacto component 
for image-blocking problems).  I'd like to set the component to navigation but I 
don't see such a component so I'm reassigning to browser-general.

FWIW, here's a clue.  Prior to the reload, the list that appears when you click 
the backbutton's arrow key starts off with the title of the previous page 
visited.  After the reload, that list starts off with three instances of "PCC 
Artwork System" (which is the title of the www.pro.com page) and then comes the 
title of the previous page visited.

Comment 4

[Asa Dotzler [:asa]]

over to session history for triage. 

Comment 5

[Radha on family leave (not reading bugmail)]

My best guess is, this is dupe of bug 58906. I would like to wait until that is 
fixed to look in to this.

Comment 6

[Radha on family leave (not reading bugmail)]

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

Comment 7

[Radha on family leave (not reading bugmail)]

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

Comment 8

[James Paige]

Radha, is this a dup of bug 66203 ?

Although, I like this one better, since it is marked 0.9.1 and bug 66203 is
marked 1.0 :)

Comment 9

[Radha on family leave (not reading bugmail)]

May be maybe not. But i think fixing this will help 66203

Comment 10

[Radha on family leave (not reading bugmail)]

Attachment: Patch to navigator.js

Comment 11

[Viswanath Ramachandran]

radha - patch doesnt look like a patch. could you take a look at it? 

Comment 12

[Alec Flett]

that's not a patch, that's a "cvs status"

Comment 13

[Radha on family leave (not reading bugmail)]

Attachment: second attempt

Comment 14

[Peter "jag" Annema]

Please don't call it BrowserReallyReload but e.g. BrowserReloadWithFlags.

Could you explain to me the (conceptual) difference between nsIWebNavigation as
implemented by session history and nsIWebNavigation as implemented by docshell?

nsDocShell.cpp seems to call through to session history in some cases, and not
in others. Should we really directly be talking to session history here, or
should docshell provide that functionality to us?

Comment 15

[Chris McAfee]

I agree with jag's comments.  r=mcafee if jag's comments are addressed.

Comment 16

[Radha on family leave (not reading bugmail)]

I will change the function name.

For other stuff,

The implementation of nsIwebnavigation  by docshell and SH are pretty similar. 
Docshell was the original implementor and holder of  SH and it was the gateway 
to history for JS. When we decided to abandon JS frame specific history, 
everything got tied to the window specific SH. That's why docshell's 
implementation of nsiWebNavigation just passes the request to SH. Embedding 
applications and viewer (nobody knows if anyone still uses it) right now use the 
docshell for history operations. I'm of the opinion that after factoring out 
certain methods from the interface, docshell need not implement nsIWebNavigation 
. 

Reload is an exception from rest of the webNavigation operations. Specifically 
because we have the "Reload frame"  feature for the browser and JS has a 
location.reload() mechanism. That is why nsDocShell::Reload() does not call in 
to  SH. But what ever nsDocShell::Reload() does is primarily to support these 2 
operations and when a new cahrset detection forces a reload.   Browser used to 
call SH directly for its history operations, but when it was decided by the 
browser team to get rid of nsBrowserInstance.cpp, it went away. I would prefer 
browser to directly call SH, simply because it is cleaner and modular. 

To sum it up, since Reload() is an exception from other history operations, the 
patch attached to this bug directly calls in to SH to avoid mis-behavior on 
frame pages. nsDocShell::Reload() is strictly to be called for "Reload frame", 
location.reload() and charset detection based reloads.

Comment 17

[rpotts (gone)]

sr=rpotts.

Comment 18

[Radha on family leave (not reading bugmail)]

This was fixed last friday

Comment 19

[James Paige]

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

Comment 20

[Claudius Gayle]

VERIFIED FIxed with 2001061415 builds on all platforms