105299 - Browser navigation (back, forward) misbehaves when location.replace() is used

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Andreas Hochsteger]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; ; rv:0.9.5) Gecko/20011012
BuildID:    2001101202

The navigation back and forward is a bit broken, when the JS command
location.replace() is used on a page to initiate a load of a new page in an
other frame.

Reproducible: Always
Steps to Reproduce:
1. Visit any site you like, or simply enter 'about:'2. Now enter
http://www.oeamtc.at/
3. Choose any story you like from the main frame
4. Go back -> this brings you 2 pages back5. Go forward -> brings you to the
homepage as expected
   -> Now the forward button is disabled, to go forward to the article 


Actual Results:  I just explained this in the "Steps to Reproduce":
* Back brings you 2 pages back
* Forward is disabled, even though it should be possible to click on it

Expected Results:  I think that's clear ;-)
* Back should bring you one page back
* Forward should bring you one page forward


The problem seems to be the JS part which is responsible to load the ad banner
in the other frame when the page gets loaded:

<!-- Start of ad loader script -->
<script LANGUAGE=JavaScript>
<!-- hide from older browsers
var fn = 'werbung';
var pn = '/netautor/pages/ads/ad.php?data=oeamtc/HP/homepage/index.ivw';
if (navigator.appName == 'Microsoft Internet Explorer' && parseInt
(navigator.appVersion) < 4) {
	window.setTimeout ('window.open(fn,pn)', 1);
} else if (window.parent.frames[fn]) {
	window.parent.frames[fn].location.replace(pn);
}
// -->
</script>
<!-- End of ad loader script -->


The real problem is the location.replace(pn) command, which seems to confuse
mozilla a bit. It should simply replace the currently displayed page in the
frame 'werbung' with the one which is provided for this page in the main frame.

Comment 1

[Johnny Stenback (:jst)]

Over to session history.

Comment 2

[R.K.Aa.]

is this what bug 103978 is about?

Comment 3

[Arne Falk]

I have similar problem on  : www.bytbil.se

In the first screen, after login, select any city to login,you should set some
search criterias.

Now if you click one of the items found, i.e. look at a car for sale and want to
go back, you cant. THERE is a special button on the page named "Tillbaka". It
works sometimes but not always.

Comment 4

[Arne Falk]

I have similar problem on  : www.bytbil.se

In the first screen, after login, select any city to login,you should set some
search criterias.

Now if you click one of the items found, i.e. look at a car for sale and want to
go back, you cant. THERE is a special button on the page named "Tillbaka". It
works sometimes but not always.

Comment 5

[Susie Wyshak]

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

Comment 6

[Susie Wyshak]

(Please change the OS to ALL...I don't know if I'm allowed to.)
Please fix this. It is annoying and unexpected behavior, that makes a user 
unproductive.

Test on MY.YAHOO.COM:

Start out NOT logged in to a Yahoo account.
1. Go to my.yahoo.com and log in
2. Click to a news story
3. Use Back button

REsult: No longer logged in
Expected: Remain logged in (as worked in earlier browser versions)

NOTE: If I log into Yahoo mail, then go to my.yahoo I *do not* get logged out 
when going to a news story then using Back button to get back to my.yahoo.

Occurs in Gecko/20011128 as well as 2002011103
______________________________
WWW.AMAZON.COM - Yesterday I experienced the same problem on a store that 
affiliates.amazon.com has. Unfortunately I can't replicate it today but here is 
what happened:

1. login at affiliates.amazon.com
2. add items to query in a text field, that will be used to generate affiliate 
ID codes.  [i attached source for this page]
3. Submit, and the results are generated 
4. Use Back button to edit/add items to the query field.

Result: Takes you go to login page. The page that had the data entry field is 
gone. 
Expected: To be able to go Back and Forward

Did not occur in IE.

Comment 7

[Susie Wyshak]

Additional info re. replicating Yahoo problem:

-I do not have "Remember my ID & Password" checked

-I have my.yahoo listed as a "Passwords never saved" in Password Manager 

-If I log in, then use the Back button I go to the website that I was on
*before* my.yahoo. So to replicate you don't actually need to go to a page
within Yahoo.

Comment 8

[Susie Wyshak]

added nsbeta1

Comment 9

[Susie Wyshak]

Changed platform to ALL. I'm using Win2K.

Comment 10

[Jaime Rodriguez, Jr.]

I've been able to reproduce this, and do have "Remember my ID & Password"
checked (but i had signed out, prior to signing back in again).

I am adding regression, because this has worked for ME in previous releases.

Of note, the lock icon is RED and broken when i log into yahoo, and unlocked
when i go to news story.

Comment 11

[Susie Wyshak]

SORRY - THE BUG 114897 WAS NOT A DUPE, AND I REMOVED KEYWORDS FROM THIS BUG AND
PASTED ALL RELEVANT INFO INTO THE OTHER ONE :-( Sorry for the mess.

Comment 12

[Radha on family leave (not reading bugmail)]

The problem here is 2 page loads happen when a  news story in the right frame is
clicked. The news is loaded in the right frame, and then an ad is  replaced in
another subframe(using location.replace). When going back, session history
recognises the  latest load which is the change in the ad, and loads it. But not
the previous load, which is the change in the news story. This is the same
problem as informer2.comdirect.de described in bug 88984. 

Comment 13

[Radha on family leave (not reading bugmail)]

Attachment: Patch to nsSHistory.cpp

This patch takes care of this bug and the site informer2.comdirect.de referred
in bug 88684

Comment 14

[Alec Flett]

Comment on attachment 70401 [details] [diff] [review]
Patch to nsSHistory.cpp

this looks fine, but the  indenting is all messed up in CompareFrames (just
look at the patch itself, you'll see what I mean)

I find it a little disturing that CompareFrames is causing a page load - maybe
it should be called LoadEntryInMatchingFrame() or something?


sr=alecf with the indenting cleaned up - your call on the function name.

Comment 15

[Radha on family leave (not reading bugmail)]

Attachment: patch with indents cleaned up per alec's suggestion.

Comment 16

[Adam Lock]

Comment on attachment 70781 [details] [diff] [review]
patch with indents cleaned up per alec's suggestion. 

r=adamlock

Comment 17

[Mike Shaver (:shaver emeritus)]

Before we approve this, can we hear what it does to Tp numbers?

Comment 18

[Radha on family leave (not reading bugmail)]

The patch adds additional cycles to the for loop, but only numeric comparison is
eventually done. No string comparisons or allocations are done. This patch also
partially fixes my other 0.9.9 bugs 88684 and 124427.

Comment 19

[Radha on family leave (not reading bugmail)]

OK.  I used the nsITimelineService to time the changes I specifically made to
the function.  As mentioned earlier, this change only affects back/forward/Go
with in a frameset. For example, if you
a) visit warp.mcom.com
b) Click on QA in the left frame
c) click back.

The code comes in to effect only in c).  The code does not become effective, if
back/forward leads to a change in the root docshell or while loading a frameset
page fresh.

To specifically time the effect of additional cycles through the for loop, I
created a test case which has 3 framesets and 2 out of 3 framesets have 2
additional children.  I loaded a new page inone of the earlier frames.

Previously, (without my patch) the function would be called recursively 3 times.
 The total time taken by the recursive function + a call to
nsDocShell::LoadURI() took 10 ms. (average of 3 runs).

With my patch the function will be called recursively for 6 times, The total
time taken by the recursive function + a call to nsDocShell::LoadURI took only
about 6.66 ms. (average of 3 runs).

Ironically, I find that the latter run(with my patch)  is faster than the
current code. I discussed this with dp. Me and him tend to believe that the
following could be the reason. But it is hard for either of us to explain.

a) The current  function has 2 out parameters that are interfaces.

nsSHistory::CompareSHEntry(nsISHEntry * aPrevEntry, nsISHEntry * aNextEntry,
nsIDocShell * aParent,
                   nsIDocShell ** aDSResult, nsISHEntry ** aSHEResult)

The 2 out parameters aDSResult and aSHEResult are addrefed with in the function,
before returning.

This has been avoided in the new implementation. The new function looks like  this,

nsSHistory::CompareFrames(nsISHEntry * aPrevEntry, nsISHEntry * aNextEntry,
nsIDocShell * aParent, long aLoadType, PRBool * aIsFrameFound)

The operation that was done outside the function (a call to
nsDocShell::LoadURI()) using the return values in the former case is now being
done inside the function.  I believe that the addreff could be contributing to
the additional time in the former case. But when I tried to specifically time
the addref part, it turned out to be less than 10 ms, so I didn't get a exact
number from nsITimelineService. (nsITimelineService provides a value of 0.00 if
the time taken by the call is < 10 ms).

I hope this data  will help the drivers make a decision in approving the patch,

Thanks,
Radha

Comment 20

[Mike Shaver (:shaver emeritus)]

That's great analysis, and I'm heartened that you put it in, but the description
of the change (fully recur in frameset hierarchies) makes it pretty clear that
it'll have different effects on deeper framesets.  I don't understand why you're
not just running the pageload tests and reporting back.  You're inside the
firewall, so the instructions at
http://www.mozilla.org/performance/tinderbox-tests.html should work fine for
you.  It's quick, it's easy, and it's what drivers asked for.  Is there some
reason that you can't run them, or do you just not want to?

Comment 21

[Radha on family leave (not reading bugmail)]

I thought, specific performance evaluation of my code will give better data
about my patch than running the tp tests. I don't believe running the tp tests
will help anyone in deciding the performace effects of my code,  since my code
will not get invoked by the tests at all.  Deeper larger framesets may result in
additional iterations through the loop. But there will be a difference between
current code and my patch *only if* the url change happened in  one  of the
frames that are on the top of the tree. If the url change happend in the last
child of the last frame, there will be no difference between current code and my
patch. 

It looks like there is no way I can get a a= unless I run the tp tests. I will
post that data when I'm in the campus tomorrow. 

Comment 22

[Radha on family leave (not reading bugmail)]

Pageload tests are at http://cowtools.mcom.com/page-loader/report.pl?id=3C7B2EFA35

Comment 23

[Radha on family leave (not reading bugmail)]

Pageload tests for  optimised linux build is at
http://cowtools.mcom.com/page-loader/report.pl?id=3C7B3A5BFF.

The previous chart reported in comment #22 was for an optimised windows build. 

Comment 24

[Mike Shaver (:shaver emeritus)]

Comment on attachment 70781 [details] [diff] [review]
patch with indents cleaned up per alec's suggestion. 

a=shaver for 0.9.9.

BTW: providing only _one_ set of page load tests doesn't really help us figure
out what the performance _change_ from a patch is, does it?

Comment 25

[Radha on family leave (not reading bugmail)]

Fix checked in.