Summary from bug 602256 comment 19: Simple STR: 1. Load the page http://download.oracle.com/javase/7/docs/api/ 2. In the address bar append an anchor like http://download.oracle.com/javase/7/docs/api/#bar 3. Click one of the classes in the bottom left frame like "AbstractAction" From here going back doesn't revert the inner fram to its old content it only changes the url back to that at step 1. Pressing forward only changes the url to that in step 2 again. At step 2 we should be cloning the child shentry entries from the main shentry rather than discarding them. Bug 602256 will fix this case for pushState, all that needs to be done here is to do the same for anchor navigation

We would need the changes in bug 602256, but also we would need to change replace loads in subframes that are anchor scrolls to not remove child shentries. Dave, do you want to take this, or should I?

Feel free to take it

I'm Huiyu Wang(hwang@mozilla.com) and working for Mozilla Beijing Office. I'd like to check with you status of this bug fixing. Since this bug causes some problems on the web page China Construction Bank,which is the second largest bank in China. We have been working with them in the past two year for support Firefox. Now this bug is kind of only one left over. Thank you very much for your understanding and the support !

The status is that I plan to fix this right after Firefox 4 ships, at the moment. If you would like, I could post a patch for you to test against the China Construction Bank site in the meantime, to make sure it fixes the issues you're seeing there. Let me know, please?

Thank you very much. Ok,if it's not too much work for you. And, if it's possible we hope that code will also be commit into 3.6.1x (x>=4).

I don't think we want to change this code on stable branches. I don't plan to fix it in 4.0.x either; just in 5.

OK. In that case it's also ok if you just give us a patch.

Hi Boris, If you can send us the patch, we'd like to give a try. Thanks a lot ! Hong

Yes, this is on my list to do ASAP. I've sort of been swamped with last-minute Firefox 4 stuff for a bit here....

Okay, Thanks !

The test is just a copy of the test from bug 602256 with some minor modifications. The rest should be pretty self-explanatory

I pushed that patch to try; the builds should appear in http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/bzbarsky@mozilla.com-42c87b545468 once they're done.

Comment on attachment 520300 [details] [diff] [review] Fix >@@ -5877,17 +5879,17 @@ nsDocShell::OnStateChange(nsIWebProgress > parent->GetCurrentSHEntry(getter_AddRefs(entry), &oshe); > static_cast<nsDocShell*>(parent.get())-> > ClearFrameHistory(entry); > } > } > > // This is a document.write(). Get the made-up url > // from the channel and store it in session history. >- rv = AddToSessionHistory(uri, wcwgChannel, nsnull, >+ rv = AddToSessionHistory(uri, wcwgChannel, nsnull, PR_FALSE, > getter_AddRefs(mLSHE)); Please comment why PR_FALSE >@@ -8320,17 +8323,17 @@ nsDocShell::InternalLoad(nsIURI * aURI, > /* This is a anchor traversal with in the same page. > * call OnNewURI() so that, this traversal will be > * recorded in session and global history. > */ > nsCOMPtr<nsISupports> owner; > if (mOSHE) { > mOSHE->GetOwner(getter_AddRefs(owner)); > } >- OnNewURI(aURI, nsnull, owner, mLoadType, PR_TRUE, PR_TRUE); >+ OnNewURI(aURI, nsnull, owner, mLoadType, PR_TRUE, PR_TRUE, PR_TRUE); Please document why the new PR_TRUE >@@ -9518,17 +9521,17 @@ nsDocShell::OnLoadingSite(nsIChannel * a > // If this a redirect, use the final url (uri) > // else use the original url > // > // Note that this should match what documents do (see nsDocument::Reset). > NS_GetFinalChannelURI(aChannel, getter_AddRefs(uri)); > NS_ENSURE_TRUE(uri, PR_FALSE); > > return OnNewURI(uri, aChannel, nsnull, mLoadType, aFireOnLocationChange, >- aAddToGlobalHistory); >+ aAddToGlobalHistory, PR_FALSE); Some comment why PR_FALSE >@@ -9776,17 +9779,17 @@ nsDocShell::AddState(nsIVariant *aData, > nsCOMPtr<nsISHEntry> newSHEntry; > if (!aReplace) { > // Save the current scroll position (bug 590573). > nscoord cx = 0, cy = 0; > GetCurScrollPos(ScrollOrientation_X, &cx); > GetCurScrollPos(ScrollOrientation_Y, &cy); > mOSHE->SetScrollPosition(cx, cy); > >- rv = AddToSessionHistory(newURI, nsnull, nsnull, >+ rv = AddToSessionHistory(newURI, nsnull, nsnull, PR_TRUE, > getter_AddRefs(newSHEntry)); Add some comment why PR_TRUE > nsresult > nsDocShell::AddToSessionHistory(nsIURI * aURI, nsIChannel * aChannel, >- nsISupports* aOwner, nsISHEntry ** aNewEntry) >+ nsISupports* aOwner, PRBool aCloneChildren, >+ nsISHEntry ** aNewEntry) > { > NS_PRECONDITION(aURI, "uri is null"); > NS_PRECONDITION(!aChannel || !aOwner, "Shouldn't have both set"); > > #if defined(PR_LOGGING) && defined(DEBUG) > if (PR_LOG_TEST(gDocShellLog, PR_LOG_DEBUG)) { > nsCAutoString spec; > aURI->GetSpec(spec); >@@ -9999,19 +10003,17 @@ nsDocShell::AddToSessionHistory(nsIURI * > if (expTime <= now) > expired = PR_TRUE; > } > if (expired) > entry->SetExpirationStatus(PR_TRUE); > > > if (root == static_cast<nsIDocShellTreeItem *>(this) && mSessionHistory) { >- // Bug 629559: Detect if this is an anchor navigation and clone the >- // session history in that case too >- if (mLoadType == LOAD_PUSHSTATE && mOSHE) { >+ if (aCloneChildren && mOSHE) { > PRUint32 cloneID; > mOSHE->GetID(&cloneID); > nsCOMPtr<nsISHEntry> newEntry; > CloneAndReplace(mOSHE, this, cloneID, entry, PR_TRUE, getter_AddRefs(newEntry)); > NS_ASSERTION(entry == newEntry, "The new session history should be in the new entry"); > } Some comment before 'if' would be great. I need to review this properly by looking at source code so that I see more context.

I'll add some comments, sure. Do you want to see those before you finish reviewing?

Thanks a lot ! We will give a try on the build on Monday and keep you posted.

Hi Boris, I've tested your patch for Bug#629559 with the case of Bug#634873, which is duplicated of Bug#629559. It seems haven't effect. Could you have a look at it? Thank you very much.

That's... odd, since I quite explicitly tested this patch on the testcase in bug 634873 and it fixed that testcase for me. I just tried again with the try build from comment 13 on Mac, and everything works correctly there. Are you sure you tested the try build?

Yes, your try build on Windows is also OK. Comment 17 is the test result on my local build: first I updated the code of mozilla-central, then merged your modification and build. Maybe it's not a good way to verify a patch.

It's a fine way, but it sounds like either the merge or the build didn't do the right thing.... When you run the resulting build, what does about:buildconfig show for the changeset it's built from? Does that match the changeset for my changes that you imported?

(In reply to comment #15) > I'll add some comments, sure. Do you want to see those before you finish > reviewing? Sorry for the delay. Yes, getting the comments for reviewing would be useful.

Many thanks to Boris. Your patch works well.

Hi Boris, I'm happy to see there is already a bug reported on this and that you have fixed it. Thank you. I am a developer in Denver and our team has experienced this same bug. If you care to, here is another illustration of it -- surf to this page and follow the three simple steps to see it: http://www.marylandmoths.com/backbug/index.html We've seen this on FF Mac OS X 3.6.16, FF Windows 3.6.13, and FF Mac OS X 4.0. FF 5.0 is a long time to wait for this fix, and the large customers we support take years to upgrade to new software after it is released. So if we wait 2 years for FF 5.0, then our customers wait another 2 years after that, it'll be 4 years before they can use our software. This bug is a showstopper for delivering our newest product on Firefox. We are fine to deliver on IE as IE doesn't have this bug, but we all prefer Firefox and wish more customers would switch to that. Is there any way this bug could be backported to Firefox 3.x and 4.x? Thank you in advance for reading my comment. Steve

I meant to say "Is there any way this bug *fix* could be backported to Firefox 3.x and 4.x?" The bug is already there :-) Thank you.

Steven, Firefox 5 (or whatever release number this fix will ship in) is slated for June 2011. So you're not going to be waiting for two years for it to ship. I can't speak to your users, of course. Unfortunately, there's no good way to backport this to 3.x and 4.x without breaking binary-compatibility promises we make in stable releases (to say nothing of stability risk from taking any changes to this code on stable branches).

Thank you for the response Boris. I think we'll be able to work with that.

Comment on attachment 521558 [details] [diff] [review] Now with more comments Looks good to me