52670 - URLs get loaded into wrong frames on reload

Status: VERIFIED FIXED

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

Description

[Jerry Baker]

1) Load the URL in Mozilla.
2) When it has finished loading, press reload.
3) Observe.

Sorry I can't be more detailed, but I don't even know how to describe this. I 
will attach a screenshot.

Comment 1

[Jerry Baker]

Attachment: See weird layout

Comment 2

[R.K.Aa.]

use the menus there for a while, and the left menu frame will also be filled
with little white/olive colored pieces of "corner" gifs. Saw that in last
sundays build. Frame bug perhaps.

Comment 3

[Kevin McCluskey (gone)]

Dividing up Claytons bugs to triage

Comment 4

[Kevin McCluskey (gone)]

I see the bad behavior on WINNT with todays build. It only happens on reload.
hitting return in the URL bar works fine.

Is this bug cache related?

Reassigning to buster.

Comment 5

[buster]

I just pulled and build (debug) on WinNT, and I don't see this at all.  Kevin, 
did you test commercial or debug build?  Chris P:  can you verify on Win2k or 
any other windows platform?

Comment 6

[Jesse Ruderman]

Fun things to try:
- Cover the page with another window
- Drop down the dropdown listbox: when I remove the listbox, I see parts of the 
tabs but with the colors messed up.

Comment 7

[buster]

ok, I see the bug now.  nice!  will investigate

Comment 8

[buster]

cc-ing eric because the document is a big frameset.  have you seen anything like
this before?

Comment 9

[buster]

changed summary, added RTM nomination, assigning to pollmann.
After the initial successful load, what seems to be happening is urls are being 
loaded into frames somewhat randomly.  Hit the reload button multiple times, and 
the url in any given frame may eventually change.  I don't know if this is an 
HTML Frames issue, a session history issue, or a cache issue (unlikely.)

I'm not 100% sure we need this for RTM, but we really need to understand it 
better.  Is this a rare case, or a common case?  What's special about this 
frameset?

Comment 10

[Eric Pollmann]

I'll take a look at this - there are a number of major regressions in form
submission and frameset loading assigned to me right now :S

Comment 11

[Eric Pollmann]

See also bug 49857

Comment 12

[buster]

Eric: I'm available to help.  Let me know if there is anything I can do to help 
you track down these regressions.

Comment 13

[buster]

bug 46646 and bug 49857 sure sound like duplicates.
based on the frequency of occurance and the lack of a workaround (once the page 
is horked, there's no obvious way to unhork it), and the fact that it is a 
regression, I strongly recommend rtm++.

Comment 14

[Eric Pollmann]

At least some of these could be related to bug 53708 / bug 53975.  In those
cases, session history is detecting that the load should come from history when
it shouldn't and pre-empting the load with a previous URL.  I've duped 53975 on
53708, and assigned that one to Nisheeth, since he's working on bug 48382, which
is almost assuredly also related.

It would be a developer's dream-come-true if one fix took care of all of these..
:)

Comment 15

[Eric Pollmann]

See bug 53708 for a reduced test case - the problem there lies in the logic in
nsDocShell::LoadURI where it detects if the load should come from session
history or not (false detection that it should come from session history)

Comment 16

[Eric Pollmann]

This looks like a problem with the order frames are stored in session history.

Initial page:
  http://www.securityfocus.com/
   a) http://www.securityfocus.com/frames/empty.html?&_ref=119562203
   b) http://www.securityfocus.com/frames/?&_ref=119562203
     0) http://www.securityfocus.com/frames/logo.html
     1) http://www.securityfocus.com/frames/ad.html?group=home
     2) http://www.securityfocus.com/frames/top.html?focus=home
     3) http://www.securityfocus.com/focus/home/menu.html
     4) http://www.securityfocus.com/frames/upper_left.html
     5) http://www.securityfocus.com/frames/upper_edge.html
     6) http://www.securityfocus.com/frames/upper_right.html
     7) http://www.securityfocus.com/frames/left_edge.html
     8) http://www.securityfocus.com/focus/home/home.html
     9) http://www.securityfocus.com/frames/right_edge.html
     10) http://www.securityfocus.com/frames/lower_left.html
     11) http://www.securityfocus.com/frames/lower_edge.html
     12) http://www.securityfocus.com/frames/lower_right.html

Reload (URLs come from session history):
  http://www.securityfocus.com/
   a) http://www.securityfocus.com/frames/empty.html?&_ref=119562203
   b) http://www.securityfocus.com/frames/?&_ref=119562203
     0) http://www.securityfocus.com/frames/logo.html
     1) http://www.securityfocus.com/frames/ad.html?group=home
     2) http://www.securityfocus.com/frames/top.html?focus=home
     3) http://www.securityfocus.com/focus/home/menu.html
     4) http://www.securityfocus.com/frames/upper_left.html
     5) http://www.securityfocus.com/frames/upper_edge.html
     6) http://www.securityfocus.com/frames/upper_right.html
     7) http://www.securityfocus.com/frames/left_edge.html
     8) http://www.securityfocus.com/frames/lower_left.html
     9) http://www.securityfocus.com/frames/right_edge.html
     10) http://www.securityfocus.com/focus/home/home.html
     11) http://www.securityfocus.com/frames/lower_edge.html
     12) http://www.securityfocus.com/frames/lower_right.html

Notice how the URLs for frames 8 and 10 are swapped in the session history load.  
I stepped though this in a debugger, and the mChildOffset seems to be okay for 
the frame when loading the page from session history, so it must be when the 
page is initially loaded that we store the wrong URL at the wrong offset in 
session history.  Handing this to Radha...

Comment 17

[Eric Pollmann]

Yep, here's the bug:

NS_IMETHODIMP
nsSHEntry::AddChild(nsISHEntry * aChild, PRInt32 aOffset)
{
    NS_ENSURE_TRUE(aChild, NS_ERROR_FAILURE);

	NS_ENSURE_SUCCESS(aChild->SetParent(this), NS_ERROR_FAILURE);
	PRInt32 childCount = mChildren.Count();
	if (aOffset < childCount)
		mChildren.InsertElementAt((void *) aChild, aOffset);
	else
	  mChildren.AppendElement((void *)aChild);
	NS_ADDREF(aChild);

    return NS_OK;
}

This method assumes that the frames come in in order, which is not guaranteed to 
be the case.  Set a breakpoint in the above method right after childCount is set 
with the condition (aOffset > childCount).  Notice a bad thing will happen in 
this case...

If the load order is: 0 3 2 1
The frames will be stored in session history as: 0 1 3 2
(If they come in really mixed up order, as happens when you step through this in 
a debugger, things get really scrambled)

One was of fixing this (may not be the most efficient as I haven't though about 
it a lot) would be to grow the list if an entry comes in before the list is long 
enough, then set the entry at the correct index instead of inserting.

Comment 18

[Eric Pollmann]

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

Comment 19

[Eric Pollmann]

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

Comment 20

[buster]

this one must be rtm+  we can't ship with reload/back whacking framesets.  bug
46828 might be a dup.

Comment 21

[don]

Radha, do you have a fix in mind for this?  Should it be re-assigned?

Comment 22

[bknotts]

I find that if I visit http://www.securityfocus.com/, and then click on the
"Vulnerabilities" link, a click on one of the links returned in the inside frame
fails to load anything. This is with a local build from CVS (Build ID: 2000092905).

Comment 23

[Randell Jesup [:jesup] (needinfo me)]

This must be RTM+'d.  Is more info really still needed?
BTW, www.nerve.com shows this problem at times too (it has 5-9 frames, depending).

Comment 24

[Radha on family leave (not reading bugmail)]

Working on a fix. Please note the [RTM NEED INFO] is a way of tracking for the 
Navigator triage team on whether this can be marked rtm+(once a patch is 
available). Please don't remove it from the status whiteboard.

Comment 25

[don]

Radha, is a fix still on the way here?

Comment 26

[Radha on family leave (not reading bugmail)]

may be tomorrow

Comment 27

[don]

Sorry, with no fix in site, I have to minus this for now.

Comment 28

[Eric Pollmann]

Attachment: suggested fix

Comment 29

[Eric Pollmann]

FWIW, I've attached a patch that does what I think might fix this bug, but I
haven't tested it or anything yet.  If this is still being considered for RTM,
it could even be made a one liner:

Index: nsSHEntry.cpp
===================================================================
RCS file: /cvsroot/mozilla/xpfe/components/shistory/src/nsSHEntry.cpp,v
retrieving revision 1.11
diff -u -r1.11 nsSHEntry.cpp
--- nsSHEntry.cpp       2000/10/13 20:49:40     1.11
+++ nsSHEntry.cpp       2000/10/16 18:51:16
@@ -213,6 +213,7 @@

        NS_ENSURE_SUCCESS(aChild->SetParent(this), NS_ERROR_FAILURE);
        PRInt32 childCount = mChildren.Count();
+       while (aOffset > childCount++) mChildren.AppendElement(nsnull);
        if (aOffset < childCount)
                mChildren.InsertElementAt((void *) aChild, aOffset);
        else

Comment 30

[Randell Jesup [:jesup] (needinfo me)]

Renominating (removing [rtm-]) now that there's a one-liner fix in hand.

Comment 31

[Radha on family leave (not reading bugmail)]

Eric, Thanks for the patch. I will try out the patch later today. I'm still
wpring on 46828.

Comment 32

[Jerry Baker]

FWIW: This patch did not seem to clear up the problem on Win2k for me.

Comment 33

[Jerry Baker]

I should add that it seems to make the first one or two reloads work, but 
continue reloading and everything gets horked the same as before. I am waiting 
for each reload to finish completely before reloading again.

Comment 34

[Eric Pollmann]

Attachment: updated patch (oops, let's try that again)

Comment 35

[Eric Pollmann]

*blush* I was still calling "Insert" so it would cause inputs like 0 3 2 1 to
come out as 0 1 null 2 null 3.  This latest patch should fix that. (2 liner)

Comment 36

[Eric Pollmann]

Okay, I just tested this latest patch on FreeBSD and it works for me!  :)

Comment 37

[Eric Pollmann]

Jerry makes a good point - what if the reload button is pressed before the load
completes?  Radha, with my patch it's possible that some of the entries in the
session history representation of the frame hierarchy could be null - would that
cause problems? (worse than already exist)

I noticed that sometimes if I reload before the page finishes, some frames won't
load at all.  However, this is also a problem before the patch.

Comment 38

[Jerry Baker]

Now that fixes it :-)

Comment 39

[don]

Moving this back up to "need info" status now that we have a patch.

Radha, should we send this bug over to Eric?

Comment 40

[Radha on family leave (not reading bugmail)]

Eric, I think you should own the bug. I have tried out the patch. It looks good. 
r=radha if you need it. Let's get a super review from rpotts or nisheeth and try 
to get it into PDT's query  today before 4:30. 

Comment 41

[Alec Flett]

sr=alecf
Also, whoever ends up checking this in, can you just add a comment explaining
why you need to extend the array in this way? I talked to radha over AIM and she
explained, but I think it would be useful to have in there.

Comment 42

[Radha on family leave (not reading bugmail)]

Some explanation on the patch:

Old code assumed that the order in which subframes come for addition in to SH 
will be the same as the order in which they actually get created. But that 
assumption is wrong. So, the new patch extends the array (by appending nsnull)to 
as many numbers required when the list is not long enough and then just replaces 
the nsnull with the SHEntry.

Comment 43

[don]

rtm+

Comment 44

[Eric Pollmann]

Attachment: patch 3 (includes comment, per super review)

Comment 45

[Eric Pollmann]

Attachment: patch 4 (includes assert per rpotts, cleanup per jar)

Comment 46

[Eric Pollmann]

Can I get one final r= and sr= to present this most recent patch to PDT?  Thanks!

Comment 47

[Eric Pollmann]

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

Comment 48

[Eric Pollmann]

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

Comment 49

[Radha on family leave (not reading bugmail)]

The latest patch looks good to me r=radha

Comment 50

[Eric Pollmann]

I have just received sr= from Rick Potts on the latest patch.  Sending back to
PDT for evaluation.

This is a very small change that fixes a session history corruption problem.  It
is low risk small, and contained.  They payoff is that any complex nested
frameset will not get mangled when clicking reload or going back/forward through
history.  There are real world test cases (this specific one has been reported
several times independantly)

Comment 51

[Eric Pollmann]

PDT: Real world test cases are:

   http://www.securityfocus.com     (Reported twice independantly)
   http://gopconvention.com
   http://cadcamonline.com

Comment 52

[Phil Peterson]

rtm++

Comment 53

[Eric Pollmann]

Fix checked in to branch and trunk.

To verify:

1) Go to www.securityfocus.com (wait for the load to complete)
2) Click the reload button

The page should loook the same as before the reload button was pressed.

Thanks!

Comment 54

[Stephan Niemz]

I can verify that this works on WinNT with build 2000101920. Thanks!

Comment 55

[Chris Petersen]

Marking verified with the Oct 19th branch build.

Comment 56

[Eric Pollmann]

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

Comment 57

[Jerry Baker]

Mass removing self from CC list.

Comment 58

[Jerry Baker]

Now I feel sumb because I have to add back. Sorry for the spam.