86330 - history fails with framed relative refs (#)

Status: VERIFIED FIXED

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

Description

[Miloslaw Smyk]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux 2.4.3 i686; en-US; rv:0.9.1+) Gecko/20010615
BuildID:    2001061521 (but it's also present in 0.9.1)

Framed URLs with relative references cause havoc in Session History.

Reproducible: Always
Steps to Reproduce:
1. Go to the URL above
2. Click java.applet in upper-left frame
3. Click AppletContext in bottom-left frame
4. Click getApplet (first item in the table) in the right frame.
5. Click back.
6. Click getApplet again.
7. notice that you can no longer use back button.

Actual Results:  Back button does nothing.

Expected Results:  Being able to use back button would be nice


I've been trying to locate the bug this weekend and in my (amateurish) opinion,
there is something broken with OSHE handling in nsDocShell.cpp. When we start
moving inside framed page by relative URLs, OSHE gets zeroed and cannot be later
used for getting ID of subframe that is necessary for
nsDocShell::CloneAndReplace(). This later causes nsSHistory::CompareSHEntry to
fail when trying to locate the subframe that's changing...

I hacked my way to fix it, but the patch is so naive that I almost surely broken
something else. Also, it's very slow and probably leaking, so I am not attaching
it here.

I think that this problem is important enought to nominate it for 0.9.2.
Also, adding it to bug #59387 may be useful.

Comment 1

[[e-mail]]

CONFIRMED, changing platform and OS to all, adding keywords regression and
mozilla0.9.2

OS: Win2k SP2
Moz Build: 20010617608 Win32 Talkback
User Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.1+)
Gecko/20010617

Comment 2

[Aaron Kaluszka]

Damn mid-air.
--
Platform/OS->All
Component->History:Session
Adding recommendations of reporter.
--
Why not include your patch just for reference, if nothing else?

Comment 3

[Miloslaw Smyk]

Attachment: Simple patch that is probably very stupid

Comment 4

[Eric Pollmann]

Handing to Radha - looks like this fix might solve some problems with framesets?

Comment 5

[Miloslaw Smyk]

First: this bug is really serious - anyone who wants to read any documentation
generated by javadoc will find mozilla useless. Not to mention any other frames
based documentation systems.

2nd: it has even nastier side-effects. What I found today is that it is enough
to click on a link that changes contents of nearby frame two times, and history
gets broken. Try this:

1. Visit http://amiga.com.pl/delfina/
2. Click "Motivation behind this page" two times (but not double-click)
3. Click "Useful tools".
4. Try to use "back" button.

In my opinion, one of the most basic and oft-used features is broken here and
I'd really hate to see next major version of NS to have it not fixed.

Please reconsider getting it done in time for 0.9.2.

I've been debugging it all day today, but I'm wasting too much time on the way
to learn things that are obvious to you...

Comment 6

[Miloslaw Smyk]

If it is not clear from my previous comment: the bug manifests itself when
contents of the frame (let's call it frame A) are replaced with the same page.
So it may be either relative reference INSIDE frame A or a link from some other
frame that loads frame A with something that was already displayed in it at that
moment.

I think that fixing the summary line would be in order...

Comment 7

[Miloslaw Smyk]

The "stupid" patch seems not so stupid after all, because:

1. Its code only kicks in in situations where unpatched mozilla would break anyway.
2. Seems to work correctly, albeit slowly.

So, if the new patch that I'm going to attach in a few minutes misbehaves,
please reconsider the "stupid" one, as I feel it's still better than current
behavior.

This new patches also appears to work in all cases, but I suppose some more
testing is necessary.

Comment 8

[Miloslaw Smyk]

Attachment: A better patch

Comment 9

[Tom Mraz]

I've tried the better patch and it's much better but not completely fixed.
This breaks yet:

1. Load the Java DOC 
2. Click on the Description in the right frame (it scrolls down).
3. Scroll up and click on Description again
4. click on org.omg.stub.java.rmi
5. try Back button - it's broken.

Comment 10

[Radha on family leave (not reading bugmail)]

Attachment: The right patch

Comment 11

[Radha on family leave (not reading bugmail)]

I have attached a patch that fixes all the problems described here. Thanks for 
the debug info.

Comment 12

[Miloslaw Smyk]

Radha, the javadoc problem seems to be fixed, but clicking twice on the same
link (described in my comment from 2001-06-21 16:16) still exhibits the same
problems. In a few minutes I'll look deeper into it.

Comment 13

[Miloslaw Smyk]

Well, let's try this one. Basically, it merges stuff Radha and I did into one
patch that correctly handles all testcases listed in comments to this bug.

Please give it some testing.

Comment 14

[Miloslaw Smyk]

Attachment: Even better patch

Comment 15

[Radha on family leave (not reading bugmail)]

With the patch I had posted on friday, I can not reproduce the problem with
visiting the same site twice ie., the problem with "clicking on description" and
the problem with amiga.com.pl/delphina. Please note that Session history will
not record  repetitive visits to the same site, ie., if you visit the same twice
consecutively, the second visit is not recorded in SH. But the back/forward
buttons continue to work right. The following lines in my patch 

if (LSHE)
   OSHE =LSHE;

takes care of the problem described here with amiga.com.pl/delphina and the
problem with clicking on description twice. 

Comment 16

[Miloslaw Smyk]

Well, as soon as I commented out my changes related to PSHE, the delfina example
was behaving badly again. Can someone else verify this, or have I just gone mad? 

Comment 17

[Tom Mraz]

I've tried both last patches.

The correct one is the later (by Miloslaw).

Try this Radha:

1. Load the Java Doc
2. Click on AbstractAction in the bottom left frame
3. Click on AbstractAction in the bottom left frame again.
4. Click on java.lang.Object in the right frame
5. Try Back button - it doesn't work with your patch.

With the latest Miloslaw's patch, everything is fine.

Comment 18

[Radha on family leave (not reading bugmail)]

Attachment: Patch by radha - second try

Comment 19

[Radha on family leave (not reading bugmail)]

The latest patch by me takes care of the problem described by Tom Mraz on
6/27/01 07:29

Comment 20

[Tom Mraz]

Very well! The last patch fixes all the testcases too! So please get it in!
I've tried some regular browsing on framed sites and it seems to be fine.

Comment 21

[Adam Lock]

r=adamlock. Patch looks fine but I would be grateful if you would take the
opportunity to rename OSHE as mOSHE and LSHE as mLSHE when you check-in.

Comment 22

[rpotts (gone)]

sr=rpotts

Comment 23

[Asa Dotzler [:asa]]

pushing out. 0.9.2 is done. (querying for this string will get you the list of
the 0.9.2 bugs I moved to 0.9.3)

Comment 24

[Tom Mraz]

The frameset history is still badly broken even with the patch.
Please see my last comment in the bug 83684.

Comment 25

[Radha on family leave (not reading bugmail)]

fix checked in

Comment 26

[Scott MacGregor]

Hey Radha, I'm confused about this bug. Why is it checked into the branch and
not the tip? 

Unfortunately this patch caused a regression on the branch w/ PDT+ bug: 89628.
It's important that we clear out mOSHE when mLSHE is null around line 4310 and
line 3509.  By not clearing out mOSHE we are causing problems in mail where end
up restoring frame state (including scroll bar postion) for every message you
click on in the message pane. 

This problem doesn't exist on the trunk because this patch isn't in the trunk. 

Comment 27

[Tom Mraz]

But the patch is BOTH on the TRUNK (rev. 1.332) and the BRANCH (rev. 1.326.2.4).

Comment 28

[Radha on family leave (not reading bugmail)]

Sorry about the delay in response. I was out of office yesterday. The patch is
checked in to trunk too, v. 1.332. THe changes were made to nsDocShell::Embed()
and nsDocShell::InternalLoad(). The change to InternalLoad() kicks in when the
url to be visited is an anchor in the same page. 

Comment 29

[Tom Mraz]

VERIFIED Linux 2001090608. (Retested all the testcases in the comments.)