Using anchor navigation in the main page breaks history tracking for inner frames

RESOLVED FIXED in mozilla5

Status

()

defect
P1
normal
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: mossop, Assigned: bzbarsky)

Tracking

Trunk
mozilla5
x86
macOS
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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?
(Reporter)

Comment 2

8 years ago
Feel free to take it
Assignee: nobody → bzbarsky
Priority: -- → P1
Blocks: 634873

Comment 3

8 years ago
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?

Comment 5

8 years ago
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.

Comment 7

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

Comment 8

8 years ago
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....

Comment 10

8 years ago
Okay, Thanks !
Duplicate of this bug: 634873
Posted patch Fix (obsolete) — Splinter Review
The test is just a copy of the test from bug 602256 with some minor modifications.  The rest should be pretty self-explanatory
Attachment #520300 - Flags: review?(dtownsend)
Attachment #520300 - Flags: review?(Olli.Pettay)
Whiteboard: [need review]
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?

Comment 16

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

Comment 17

8 years ago
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?

Comment 19

8 years ago
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.
Attachment #521558 - Flags: review?(dtownsend)
Attachment #521558 - Flags: review?(Olli.Pettay)
Attachment #520300 - Attachment is obsolete: true
Attachment #520300 - Flags: review?(dtownsend)
Attachment #520300 - Flags: review?(Olli.Pettay)

Comment 23

8 years ago
Many thanks to Boris. Your patch works well.
Attachment #521558 - Flags: review?(Olli.Pettay) → review+

Comment 24

8 years ago
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

Comment 25

8 years ago
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).

Comment 27

8 years ago
Thank you for the response Boris.   I think we'll be able to work with that.
(Reporter)

Comment 28

8 years ago
Comment on attachment 521558 [details] [diff] [review]
Now with more comments

Looks good to me
Attachment #521558 - Flags: review?(dtownsend) → review+
Whiteboard: [need review] → [need landing]
http://hg.mozilla.org/projects/cedar/rev/d6e01d649ddc
Flags: in-testsuite+
Whiteboard: [need landing] → [fixed-in-cedar]

Comment 30

8 years ago
http://hg.mozilla.org/mozilla-central/rev/d6e01d649ddc
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-cedar]
Target Milestone: --- → mozilla2.2
You need to log in before you can comment on or make changes to this bug.