Closed
Bug 52670
Opened 24 years ago
Closed 24 years ago
URLs get loaded into wrong frames on reload
Categories
(Core :: DOM: Navigation, defect, P2)
Core
DOM: Navigation
Tracking
()
VERIFIED
FIXED
M18
People
(Reporter: mozilla, Assigned: pollmann)
References
()
Details
(Keywords: regression, Whiteboard: [rtm++]Fix in hand, reviewed and approved [mpt:msdn])
Attachments
(5 files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•24 years ago
|
||
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 4•24 years ago
|
||
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.
Assignee: kmcclusk → 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?
Status: NEW → ASSIGNED
Comment 6•24 years ago
|
||
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.
cc-ing eric because the document is a big frameset. have you seen anything like
this before?
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?
Assignee: buster → pollmann
Severity: normal → major
Status: ASSIGNED → NEW
Keywords: rtm
Summary: Layout of page is completely hosed → URLs get loaded into wrong frames on reload
Assignee | ||
Comment 10•24 years ago
|
||
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
Status: NEW → ASSIGNED
Assignee | ||
Comment 11•24 years ago
|
||
See also bug 49857
Comment 12•24 years ago
|
||
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•24 years ago
|
||
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++.
Keywords: regression
Assignee | ||
Comment 14•24 years ago
|
||
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..
:)
Assignee | ||
Comment 15•24 years ago
|
||
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)
Assignee | ||
Comment 16•24 years ago
|
||
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...
Assignee: pollmann → radha
Status: ASSIGNED → NEW
Component: Layout → History
OS: Windows 2000 → All
Hardware: PC → All
Assignee | ||
Comment 17•24 years ago
|
||
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.
Assignee | ||
Comment 18•24 years ago
|
||
*** Bug 46646 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 19•24 years ago
|
||
*** Bug 52001 has been marked as a duplicate of this bug. ***
Comment 20•24 years ago
|
||
this one must be rtm+ we can't ship with reload/back whacking framesets. bug
46828 might be a dup.
Comment 21•24 years ago
|
||
Radha, do you have a fix in mind for this? Should it be re-assigned?
Whiteboard: [RTM NEED INFO]
Comment 22•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
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.
Status: NEW → ASSIGNED
Target Milestone: --- → M18
Comment 25•24 years ago
|
||
Radha, is a fix still on the way here?
Comment 26•24 years ago
|
||
may be tomorrow
Comment 27•24 years ago
|
||
Sorry, with no fix in site, I have to minus this for now.
Whiteboard: [RTM NEED INFO] → [rtm-]
Assignee | ||
Comment 28•24 years ago
|
||
Assignee | ||
Comment 29•24 years ago
|
||
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•24 years ago
|
||
Renominating (removing [rtm-]) now that there's a one-liner fix in hand.
Whiteboard: [rtm-]
Comment 31•24 years ago
|
||
Eric, Thanks for the patch. I will try out the patch later today. I'm still
wpring on 46828.
Reporter | ||
Comment 32•24 years ago
|
||
FWIW: This patch did not seem to clear up the problem on Win2k for me.
Reporter | ||
Comment 33•24 years ago
|
||
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.
Assignee | ||
Comment 34•24 years ago
|
||
Assignee | ||
Comment 35•24 years ago
|
||
*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)
Assignee | ||
Comment 36•24 years ago
|
||
Okay, I just tested this latest patch on FreeBSD and it works for me! :)
Assignee | ||
Comment 37•24 years ago
|
||
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.
Reporter | ||
Comment 38•24 years ago
|
||
Now that fixes it :-)
Comment 39•24 years ago
|
||
Moving this back up to "need info" status now that we have a patch.
Radha, should we send this bug over to Eric?
Whiteboard: [rtm need info]
Comment 40•24 years ago
|
||
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.
Assignee: radha → pollmann
Status: ASSIGNED → NEW
Assignee | ||
Updated•24 years ago
|
Status: NEW → ASSIGNED
Whiteboard: [rtm need info] → [rtm need info] fix in hand
Comment 41•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
rtm+
Whiteboard: [rtm need info] fix in hand → [rtm+]Fix in hand, reviewed and approved
Assignee | ||
Comment 44•24 years ago
|
||
Assignee | ||
Comment 45•24 years ago
|
||
Assignee | ||
Comment 46•24 years ago
|
||
Can I get one final r= and sr= to present this most recent patch to PDT? Thanks!
Whiteboard: [rtm+]Fix in hand, reviewed and approved → [rtm need info]Fix in hand, reviewed and approved
Updated•24 years ago
|
Whiteboard: [rtm need info]Fix in hand, reviewed and approved → [rtm need info]Fix in hand, reviewed and approved [mpt:msdn]
Assignee | ||
Comment 47•24 years ago
|
||
*** Bug 55941 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 48•24 years ago
|
||
*** Bug 55941 has been marked as a duplicate of this bug. ***
Comment 49•24 years ago
|
||
The latest patch looks good to me r=radha
Assignee | ||
Comment 50•24 years ago
|
||
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)
Whiteboard: [rtm need info]Fix in hand, reviewed and approved [mpt:msdn] → [rtm+]Fix in hand, reviewed and approved [mpt:msdn]
Assignee | ||
Comment 51•24 years ago
|
||
PDT: Real world test cases are:
http://www.securityfocus.com (Reported twice independantly)
http://gopconvention.com
http://cadcamonline.com
Comment 52•24 years ago
|
||
rtm++
Whiteboard: [rtm+]Fix in hand, reviewed and approved [mpt:msdn] → [rtm++]Fix in hand, reviewed and approved [mpt:msdn]
Assignee | ||
Comment 53•24 years ago
|
||
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!
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Comment 54•24 years ago
|
||
I can verify that this works on WinNT with build 2000101920. Thanks!
Comment 55•24 years ago
|
||
Marking verified with the Oct 19th branch build.
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 56•24 years ago
|
||
*** Bug 58091 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 57•22 years ago
|
||
Mass removing self from CC list.
Reporter | ||
Comment 58•22 years ago
|
||
Now I feel sumb because I have to add back. Sorry for the spam.
Component: History: Session → Document Navigation
QA Contact: chrispetersen → docshell
You need to log in
before you can comment on or make changes to this bug.
Description
•