Closed Bug 48382 Opened 24 years ago Closed 24 years ago

setting location.href onload in frames causes infinite loop

Categories

(Core :: DOM: Core & HTML, defect, P1)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: wo, Assigned: nisheeth_mozilla)

References

(
URL
)

Details

(Keywords: crash, Whiteboard: [nsbeta3++][PDTP1]ETA - 9/28)

Attachments

(1 file)

Attempted fix
(deleted), patch
Details | Diff | Splinter Review
When setting <body onload="location.href='somewhere.else'"> in a frame document, Mozilla seems to reload the current page before changing the location, thereby causing yet another onload event, which again reloads the current page, etc. - the browser is trapped in an infinite loop. Steps to Reproduce: make a frameset where one frame has the following body tag: <body onload="alert('hey');location.href='http://www.mozilla.org'> Actual Results: infinitely many "hey" alerts. Expected Results: one "hey" alert, and a change of the frame's location. (of course you don't need the alert. If you omit it the browser will just appear frozen.) This bug is new in M17, at least I can't reproduce it in M16. It is maybe related to: http://bugzilla.mozilla.org/show_bug.cgi?id=40929 and http://bugzilla.mozilla.org/show_bug.cgi?id=47279.
Not sure what's going on here yet but we don't want this in beta3, locking up in a simple case like this is not acceptable. Thanks for catching this lockup!
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: crash, nsbeta3
OS: Windows 98 → All
Hardware: PC → All
Target Milestone: --- → M18
Re-assigning to rick for more investigation. Rick, sorry about this, but Johnny is overloaded with beta3+ bugs at this point and does not have the bandwidth to do an initial investigation on this bug. Please triage this bug according to your group's priorities. Thanks!
Forgot to re-assign to rpotts. Doing so.
Assignee: jst → rpotts
Status: ASSIGNED → NEW
Whiteboard: [nsbeta3+]
*** Bug 47279 has been marked as a duplicate of this bug. ***
Bug 47279 is exactly the same scenario - frameset with a single frame containing this document: <html> <body onload="window.location='http://www.nasslan.net/kampanj/index.php'"> </body> </html> The page is reloaded in an infinite loop (which can be broken by hitting the stop button in the GUI). All of the repeat loads hit the cache (not the server).
http://blueviper.mcom.com/frames/outer.html <html> <frameset rows="100%"> <frame src="redirect.html"> </frameset> </html> http://blueviper.mcom.com/frames/redirect.html <html> <body onload="window.location='http://www.mozilla.org/'"> </body> </html>
Okay, I turned off the cache (browser.cache.enable and browser.cache.disk.enable both set to false), and we are repeatedly hitting the server asking for the page that is originally loaded in the frameset, not the page that the window.location is set to. (i.e. in this case, http://blueviper.mcom.com/frames/redirect.html instead of http://www.mozilla.org) I'm puzzled by this, because when I set a breakpoint in nsDocShell::LoadURI, we kept trying to load the new page (i.e. http://www.mozilla.org) Trying to find out where nsDocShell::LoadURI's new URL request gets changed to the old URL...
Looks like session history is taking over the load in nsDocShell::LoadURI, and insisting on the old URL...
I found a one-liner that fixes this bug: Index: nsDocShell.cpp =================================================================== RCS file: /cvsroot/mozilla/docshell/base/nsDocShell.cpp,v retrieving revision 1.186 diff -r1.186 nsDocShell.cpp 235c235 < if (!shEntry && loadType != nsIDocShellLoadInfo::loadNormalReplace) { --- > if (!shEntry && loadType == nsIDocShellLoadInfo::loadHistory) { Does that seem like a reasonable thing? (FWIW, when window.location is set, the load type is nsIDocShellLoadInfo::loadNormal) CC'ing Radha who might have some insight here.
Whiteboard: [nsbeta3+] → [nsbeta3+] fix in hand
Keywords: patch
Priority: P3 → P1
*** Bug 50708 has been marked as a duplicate of this bug. ***
The above is not a reasonable fix... Radha was kind enough to point out that it breaks session history for subframe loads in framesets. *blush*
Whiteboard: [nsbeta3+] fix in hand → [nsbeta3+]
eric, I'm a bit confused here. When load type is loadNormal (which is the case for location.href), it may enter that if() clause, but it shouldn't get any shEntry in it. When it doesn't find any shEntry(thereby a uri) uri from its parent, it should continue loading whatever was passed in. Please clarify. May be talk about this later today when you are in. Radha
Okay, what I see is this: http://blueviper/frames/outer.html When loading the inner frame the first time, we do fall into the above code (!shEntry and loadType == loadNormal). There, we get the parent, and call GetChildSHEntry. It is null at this point. After the inner frame is loaded, the javascript onload handler fires. In this handler, we set location.href to something. This calls nsDocShell::LoadURI again: #0 nsDocShell::LoadURI (this=0x8463730, aURI=0x8384d30, aLoadInfo=0x8384e00) at nsDocShell.cpp:260 #1 0x40f70d1f in LocationImpl::SetHrefWithBase (this=0x8384308, aHref=@0xbfffdda4, aBase=0x83771a0, aReplace=0) at nsLocation.cpp:398 #2 0x40f724a1 in LocationImpl::SetProperty (this=0x8384308, aContext=0x834b578, aObj=0x83785f0, aID=137856356, aVp=0xbfffe890) at nsLocation.cpp:812 #3 0x40f735aa in nsJSUtils::nsCallJSScriptObjectSetProperty (aSupports=0x8384308, aContext=0x834b578, aObj=0x83785f0, aId=137856356, aReturn=0xbfffe890) at nsJSUtils.cpp:196 #4 0x40f551cd in SetLocationProperty (cx=0x834b578, obj=0x83785f0, id=137856356, vp=0xbfffe890) at nsJSLocation.cpp:286 <JAVASCRIPT ON STACK, ...> loadType is loadNormal here because LocationImpl::SetProperty calls LocationImpl::SetHrefWithBase with aReplace set to PR_FALSE. We then proceed through the same logic, get our parent, then call GetChildSHEntry. It is not null anymore, and sets the value back to the incorrect previous value. Since the page we're loading has an onload handler set, it calls the same code over and over again in an infinite loop.
*** Bug 51127 has been marked as a duplicate of this bug. ***
*** Bug 51843 has been marked as a duplicate of this bug. ***
PDT agrees P1
Whiteboard: [nsbeta3+] → [nsbeta3+][PDTP1]
*** Bug 50566 has been marked as a duplicate of this bug. ***
*** Bug 50129 has been marked as a duplicate of this bug. ***
*** Bug 52995 has been marked as a duplicate of this bug. ***
Keywords: mostfreq
*** Bug 48127 has been marked as a duplicate of this bug. ***
So, it sounds like we are entering the "Frameset Restoration State" (FRS) too eagerly in LoadURI(...). I believe that what is happening is that when the child frame is loaded, it enters the FRS state... When it finishes loading it's OnLoad handler fires and changes location.href. This causes LoadURI to be called again. Unfortunately, since the child's parent is still loading (because the child hasn't finished its OnLoad) the child again enters the FRS state and ignores the URI being passed in. This causes the original document to be reloaded - and the cycle begins again. We need to discriminate a bit more before entering the FRS state. Perhaps, we could use the mCurrentURI member in nsDocShell as a 'hint' Since we only want to enter the FRS state for the first URI load, perhaps we should check if mCurrentURI == nsnull This would only be true for the first URI loaded in the docshell... I have no idea if this will sork in all cases... Over to nisheeth for more careful consideration :-)
Assignee: rpotts → nisheeth
PDT agrees this is needed on branch, marking nsbeta3++.
Whiteboard: [nsbeta3+][PDTP1] → [nsbeta3++][PDTP1]
That is right. This is what I think of the problem. Looks like this problem started when location.href started doing loadNormal( a change that myself and Nisheeth came up with, to fix a bunch of bugs ofcourse) instead of loadReplace. Many sites seem to use location.href in the onload handler to do a redirection. Some use it in places other than onload handler (most of our bookmark code). So, it seems that when the location.href is called in onload handler, we should treat it as loadReplace and in other places as loadNormal. Is there any way docshell can know about the onLoad handler situation?
Adding ETA to status whiteboard.
Status: NEW → ASSIGNED
Whiteboard: [nsbeta3++][PDTP1] → [nsbeta3++][PDTP1]ETA - 9/28
Rick's suggestion to check mCurrentURI makes sense. The attached patch fixes this bug. Radha, do you have a set of session history test cases that I can run to verify that this change does not break other session history codepaths?
Attached patch Attempted fix (deleted) — — Splinter Review
Eric has reviewed the patch and I've tested back/forward in frameset and non-frameset pages. At this point, I'm confident about this fix and really want it to get tested in tomorrow's builds. I've checked the patch into the branch and trunk. Marking fixed.
Really marking fixed...
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Verified with 09-29-08 on win-95. Working perfect.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: