Closed Bug 201108 Opened 21 years ago Closed 21 years ago

<BODY onLoad>, |location.href| for other frame(s) appears ignored at first

Categories

(Core :: DOM: Navigation, defect)

Product:
Core
Component:
DOM: Navigation
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.4final

People

(Reporter: sgautherie, Assigned: radha)

References

Details

(Keywords: regression, topembed+, Whiteboard: [adt2])

Attachments

(2 files)

Reduced Testcase (RTC_SG_1)
1.02 KB, application/zip
Details
quick patch to docshell
1.78 KB, patch
danm.moz
: review+
darin.moz
: superreview+
asa
: approval1.4+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.4a) Gecko/20030401
Build Identifier: Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.4a) Gecko/20030401

First of all, let's say that this is a regression which happened between
"Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.3) Gecko/20030312"
and
"Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.4a) Gecko/20030401"
!
{It's the first time that I need to install back a previous release to check for
sure :-(}

This had allways (= for *many* releases at least) been working before v1.4a.
(Netscape v4.8 and previous has allways worked too, but that may be another story.)


The relevent source code extract (from a frame) is
[
<body onload="if (top.location == self.location) {self.location =
'https://www.net751.caisse-epargne.fr/private/ident.asp';}else load3();
defaultStatus=''; return true">

<script LANGUAGE="JavaScript">
<!--	
	function load1 ()
	{
		parent.gauche.location.href='/private/vide_g.asp';
		parent.haut.location.href='/private/vide_h.asp';
	}
	
	function load2H ()
	{
		parent.gauche.location.href='/private/frame2_g.asp';
		parent.haut.location.href='/private/vide_h.asp';
	}

	function load2V ()
	{
		parent.gauche.location.href='/private/vide_g.asp';
		parent.haut.location.href='/private/frame2_h.asp';
	}

	function load3 ()
	{
		parent.gauche.location.href='/private/frame2_g.asp';
		parent.haut.location.href='/private/frame2_h.asp';
	}
//-->
</SCRIPT>
]

There is no error in the JavaScript Console;
and I can't tell if it is related to one of the many bug filed about the
"onLoad()" event, or if it is something (JavaScript !?) else which has gone wrong.


Bottom line is that it prevents me from loading the menus, which I miss, at
least because they contain the button to logout of the "site session".


Reproducible: Always

Steps to Reproduce:
(I see this on an banking site, and I can't provide my private access...)

Nevertheless, see the results below, as observed on screen and confirmed by a
network (proxy) activity capture.

Actual Results:  
In v1.4a, it's as if 'load1()' is executed, which should not.


Expected Results:  
In v1.3, it's as if 'load3()' is executed, which is expected.


Feel free to change the component and the summary as needed.

I could generate "nsHttp:5" logs if you want them.

If you have a guess on the date of regression, I'd be willing to try {for the
first time} 1+ nightly (v1.4a) builds as needed.
Keywords: regression
Summary: BODY OnLoad, in a frame, does the wrong action... (that the initial case) → BODY OnLoad, in a frame, does the wrong action... (that's the initial case)
Flags: blocking1.4b?
Is there a page this could be tested on?
Not on the actual site (private access) :-(

In that case, we're going need a lot more of the source code to see what's going
on -- use http://bugzilla.mozilla.org/attachment.cgi?bugid=201108&action=enter
please...
1. Unzip the 4 files in the same directory;
2. Load the 'index.asp.html' file; (I added 2 alert())
3. On the right frame, use 'This Frame > Reload Frame';
4. Check that the left frame has changed. (Use 'Back/Forward' to best see it)
OK, that's calling the right function but the location change is not
happening... not sure why.  jst?
Assignee: asa → dom_bugs
Component: Browser-General → DOM Level 0
OS: Windows 95 → All
QA Contact: asa → ashishbhatt
Hardware: PC → All
Re comment 3: (and addition to attachment in comment 4)

I feared you would say that;

But lucky you:
I spent some time retrieving the various files {frameset in frameset, post,
redirect: who can design such a site :-<};
Only to spend some more time narrowing down the relevent code.
You won't be disappointed: I left a few lines only :->

First conclusion is that 'load3()' is allways executed, only it fully work on
Frame Reload only.

NB: On the actual site, even the Frame Reload workaround won't fix anything ...
I just hope that fixing the local testcase behaviour will fix the actual site
behaviour too.!. (== this will have to be checked with a HTTP server too, in a
second time.)

Netscape v4.8: allways updates (between the 2 alert())
Mozilla v1.3: allways updates (after the 2nd alert())

Now, I can let you try and find at which build, and why, this bug appeared :->
Keywords: 4xp
Summary: BODY OnLoad, in a frame, does the wrong action... (that's the initial case) → BODY OnLoad, in a frame ... location.href does nothing (in the reported case)
Blocks: 201159
Blocks: 201336
Blocks: 201337
Upping severity.  This is getting reported a _lot_ with 1.4a.

When I get home tonight I'll look for when the regression happened.
Severity: major → critical
OK, this broke between build 2003-03-13-08 (which works fine) and build
2003-03-14-05 (which is broken).  Looking at the checkins in that window...
No longer blocks: 201336
No longer blocks: 201337
Blocks: 200676
No longer blocks: 200676
OK, locally backing out bug 166736 fixes this bug on the bug this blocks.

Over to session history, ccing the various reviewers and such.  I bet we drop
the location change on the floor because we're in the middle of a load or some
such...
Assignee: dom_bugs → radha
Component: DOM Level 0 → History: Session
QA Contact: ashishbhatt → petersen
I wouldn't want the other patch to bug 166736 to be backed out, since that takes
care of a more common basic problem. I will look into this when I get a chance. 
Right, I was not saying we should back it out; just stating what led me to think
it was definitely the culprit.  ;)

Thanks for looking into this, Radha.
Addition to comment 6 (and confirmation of comment 11):

On the actual 'net751' site,
The Reload trick is helpless;
But I discovered that using Back, does its job AND the frames then display there
previously intended content :-)

This seems definately related to bug 166736 about "History: Session".
Summary: BODY OnLoad, in a frame ... location.href does nothing (in the reported case) → BODY onLoad, location.href for other frame(s) appears ignored at first
Precision on comment 12:

The steps are:
(Login) -> 1st frameset display with some empty frames -> click on a link to
change the frame which loaded fine -> new frame, still empty others -> Back ->
get all initials frames with there intended content :-)

Re comment 9:

Which bug report is "fixed" by the back out ?
The current one (201108) and/or bug 201159 ?

If both, bug 201159 could be marked as duplicate;
If one only, then we would know that there are at least two issues to fix.

> Which bug report is "fixed" by the back out ?

Both.  But since the fix will _not_ be to back it out, I'd like to keep the bugs
separate in case we end up fixing one but not the other (hopefully, that will
not happen).
Blocks: 202258
Flags: blocking1.4b? → blocking1.4b+
If you want another site exhibiting this problem:

http://xa2.loran.com/nm/find/
login=demo
password=guide


I have reverted back to moz 1.3 for now.
adt: nsbeta1+/adt2
Keywords: nsbeta1 → nsbeta1+
Whiteboard: [adt2]
Flags: blocking1.4b-
Flags: blocking1.4b+
Flags: blocking1.4+
ADT: Nominating topembed
Keywords: topembed
[Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.4b) Gecko/20030507]

Bug still there. (with v1.3 profile, at least)
Keywords: topembed → topembed+
[Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.3.1) Gecko/20030425]

v1.3.1 works fine, as v1.3 does. (with v1.3 profile, at least)

so this might be one of the few remaining higher visability regressions left
open from the 1.0 branch that need to be cleaned up for 1.4 final.  what do we
need to do here?  who can help out?
I think rahda is out until the end of the month.
bzbarsky (or anyone who has a latest tree and interested in this bug) , I need
your help here. Looking thro' the changes to the other bug, it looks like it is
this following segment from the patch to bug 166736, that is causing this bug:

@@ -648,31 +646,28 @@
                     // This is a pre-existing subframe. If the load was not
originally initiated
                     // by session history, (if (!shEntry) condition succeeded)
and mCurrentURI is not null,
                     // it is possible that a parent's onLoadHandler or even
self's onLoadHandler is loading 
-                    // a new page in this child. Check parent's and self's busy
status and if it is, 
+                    // a new page in this child. Check parent's and self's
onLoadHandler flag  and if it is set,
                     // we don't want this onLoadHandler load to get in to
session history.
-                    PRUint32 parentBusy=BUSY_FLAGS_NONE, selfBusy =
BUSY_FLAGS_NONE;
-                    parentDS->GetBusyFlags(&parentBusy);                    
-                    GetBusyFlags(&selfBusy);
-                    if (((parentBusy & BUSY_FLAGS_BUSY) || (selfBusy &
BUSY_FLAGS_BUSY)) && shEntry) {
-                        // we don't want this additional load to get into
history, since this
-                        // load will automatially happen everytime, no matter
how the page is loaded.
+                    PRBool parentInOnLoadHandler=PR_FALSE, selfInOnLoadHandler
= PR_FALSE;
+                    parentDS->IsExecutingOnLoadHandler(&parentInOnLoadHandler);
                   
+                    IsExecutingOnLoadHandler(&selfInOnLoadHandler);
+                    if ((parentInOnLoadHandler || selfInOnLoadHandler) &&
shEntry) {
                         loadType = LOAD_NORMAL_REPLACE;
                         shEntry = nsnull; .

Can you just undo this part of my checkin and see if it fixes the problem. It is
99% the above segment that is causing this bug OR the following one:


@@ -615,14 +615,12 @@
                     if (shEntry && (parentLoadType == LOAD_NORMAL ||
parentLoadType == LOAD_LINK)) {
                         // The parent was loaded normally. In this case, this
*brand new* child really shouldn't
                         // have a SHEntry. If it does, it could be because the
parent is replacing an
-                        // existing frame with a new frame, probably in the
onLoadHandler. We don't want this
+                        // existing frame with a new frame, in the
onLoadHandler. We don't want this
                         // url to get into session history. Clear off shEntry,
and set laod type to
                         // LOAD_BYPASS_HISTORY. 
-                        PRUint32 parentBusy=BUSY_FLAGS_NONE;
-                        parentDS->GetBusyFlags(&parentBusy);
-                        if (parentBusy & BUSY_FLAGS_BUSY) {
-                            // The parent is still busy. We most likely got here
-                            // through onLoadHandler. 
+                        PRBool inOnLoadHandler=PR_FALSE;
+                        parentDS->IsExecutingOnLoadHandler(&inOnLoadHandler);
+                        if (inOnLoadHandler) {
                             loadType = LOAD_NORMAL_REPLACE;
                             shEntry = nsnull;

Can someone try this out and update me. I don't have a latest tree nor
consistent cycles to verify the above. Thanks.
yes, backing out the first chunk in comment 23 fixes this bug.
This patch backs off part of the checkin for bug 166736. I have not tested it
nor successfully compiled it in my outdated tree. I would  like someone to
stress test it for me. I think this will take care of both this bug and 166736.
Thanks.
Comment on attachment 123995 [details] [diff] [review]
quick patch to docshell

I don't grok all the subtleties going on here. The offending patch (among other
things) set loadType to LOAD_NORMAL_REPLACE if a URI load was fired off in a
subframe a little later in the load process (and introduces a whole new
accessor which seems to me to be merely another flavour of GetBusyFlags and I
find that confusing). This patch reverts that part of the offending patch. I'm
guessing this patch partially regresses bug 166736 and the underlying problem
is that we're inappropriately multiplexing LOAD_NORMAL_REPLACE. We may need a
new flag.

Regardless, this patch behaves as expected (in a good way) in every test I
tried. In load handlers I've: called focus, called alert, accessed/mutated dom
elements and window properties, fired off an image load, opened popup windows,
sized the window, set a timer, (from a frame) called a method on the parent to
document.write [ into another frame, and into the calling frame HTML containing
a load handler of its own ]. All good. I say check it in. (I can check it in
for vacationing Radha.)
Attachment #123995 - Flags: review+
...not to mention the patch also fixes the testcase mentioned in this bug. And I
just realized I was wasting my time with most of those other tests. The effect
of this patch should be very narrow: URI loads fired off in load handlers, which
is this bug's complaint. Well that seems to work, too. My review+ remains.
who can super-review this?
Comment on attachment 123995 [details] [diff] [review]
quick patch to docshell

sr=darin
Attachment #123995 - Flags: superreview+
Comment on attachment 123995 [details] [diff] [review]
quick patch to docshell

a=asa (on behalf of drivers) for checkin to 1.4
Attachment #123995 - Flags: approval1.4+
Dan, Thanks for all the stress testing. Can you check it in for me?
checked in to trunk (23 May) and 1.4 branch (27 May)
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.4final
Keywords: fixed1.4
[Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.4) Gecko/20030529]

(As reporter) I verified that this bug is fixed in v1.4rc1,
for both real site (Description) and testcase (comment 6):
v1.4rc1 works fine, as v1.3.1 does.
Summary: BODY onLoad, location.href for other frame(s) appears ignored at first → <BODY onLoad>, |location.href| for other frame(s) appears ignored at first
No longer blocks: 201159
*** Bug 201159 has been marked as a duplicate of this bug. ***
No longer blocks: 202258
Verified on Macho /Win32 trunk and branch builds.
2003-06-06-05 - Macho branch,Win32 branch
2003-06-06-08 - Macho trunk, Win32 trunk
Status: RESOLVED → VERIFIED
Keywords: fixed1.4 → verified1.4
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.

Attachment

General

Creator:
Created:
Updated:
Size: