Closed Bug 58613 Opened 24 years ago Closed 21 years ago

"last page visited" cannot handle framed pages properly

Categories

(Core Graveyard :: History: Global, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Future

People

(Reporter: bugzilla, Assigned: iannbugzilla)

References

()

Details

(Keywords: fixed1.4.1, Whiteboard: relnote-user)

Attachments

(1 file, 2 obsolete files)

occurs on all platforms --tested using 2000.10.30.09/8-n6 opt commercial branch
bits. [does this occur on the trunk?]

1. open up Prefs dialog, select Navigator category.
2. under the "When Navigator starts up, display" section, select the "Last page
visited" radio button.
3. click OK to save and dismiss Prefs.
4. go to any page with frames, eg:
	http://www.faqs.org/
	http://www.wired.com/
5. don't click in any of the frames (this assumes that the main frameset is
active, not any particular frame --afaik).
6. quit the app.
7. restart the app.

expected: upon startup, the main frameset should be loaded.

result: instead, one of the frames [not the main frameset] is loaded. at this
point i'm unsure what the [incorrect] algorithm is for choosing which frame to
load --at first i thought it'd be the second frame listed in the frameset --but
i repeated the recipe above several times [each time making a point to *not*
bring any frame into focus], but sometimes i'd get the first frame, sometimes
the second one --there was only *one* [not repro reliably] time where the main
frameset did load as expected --but that was alas only once.

cc'ing petersen and pollman [since they're the main contacts for frames], in
case they might have further insight as to why this might be happening. thx!
adding kw/status notes --as i doubt this'll get fixed on the branch any time
soon. ;(

again, if anyone has seen a pattern as to how this problem is caused, do let me
know --at least then perhaps a workaround relnote could be added here...
Keywords: relnoteRTM
Whiteboard: relnote-user
works fine in 4.7x. also adding correctness.
Keywords: 4xp, correctness
I don't know anything about how this feature is implemented, but as a stab in
the dark, I do know that the order in which frames complete loading is not
guaranteed to be the order they are listed inside the frameset.  That is, this:

main.html:
  <frameset rows = "*,*,*">
    <frame name=one src=one.html>
    <frame name=two two.html>
    <frame name=three src=three.html>
  </frameset>

Could load just as easily:
  main.html, one.html, two.html, three.html    -as-
  main.html, two,html, three.html, one.html

If we are just storing the last page that finishes loading, this could be
randomly any one of the frames in a frameset depending on network delays.  Also,
even the containing frame could happen to finish loading last - through that
would be rare and explain why it was only seen once.

Where in our code is the "last page" stored?  I think if this was a load
observer, this would guarantee that the containing frameset was marked as loaded
last every time, but I'm not 100% sure that this works as it should. ?
So it should load the entire frameset, not just a single frame -- right?

I'm still looking for where we set the pref.  Meanwhile, the spelling 
of "visited" here baffles me: 
http://lxr.mozilla.org/seamonkey/source/xpfe/browser/src/nsBrowserInstance.cpp#2
463
oh.  that is where we set it, I think.

<slaps forehead>
OK, I think Eric's right.  We just use the last page added to the global 
history:

http://lxr.mozilla.org/seamonkey/source/xpfe/components/history/src/nsGlobalHist
ory.cpp#331

We'll have to make an exception if it's a framed page, I guess.
when you wish upon a star...
Keywords: nsbeta1
This probably depends on an as-yet-unreported bug which bug 25909 will also 
depend on.
nav triage: radha - can you please dup or depends this bug. 
Assignee: matt → radha
THis is a actual problem that should be fixed in Global History. Basically pages 
are added to global History as they are loaded by docshell and in a frameset 
page, the last frame loaded gets marked as last page visited. Global history 
needs to be smart about differentiating a  regular page and a subframe and not 
add subframes as last visited page. Giving to alecf who right now owns global 
history. 
Assignee: radha → alecf
Component: Preferences → History
Target Milestone: --- → mozilla0.9.1
moving to mozilla0.9.1
QA Contact: sairuh → claudius
Status: NEW → ASSIGNED
Component: History: Session → History: Global
Target Milestone: mozilla0.9.1 → mozilla1.0
*** Bug 56134 has been marked as a duplicate of this bug. ***
Marking nsbeta1+. Leaving mozilla1.0 milestone until I can figure out where to
schedule this.
Keywords: nsbeta1nsbeta1+
*** Bug 81378 has been marked as a duplicate of this bug. ***
I figured out a way to solve this on the global history side of things, but it
requires that I fix the way that history gets notified when a page is loaded.
basically I need history to be waiting on a page load (bug 71482) and then when
the page is loaded, we check if it's a subframe or not - if not, then we set the
last page visited to that page..
Depends on: 71482
Filed related bug 84615, regarding "last page visited" opening to internal
popups like the 'get plugin' window.
actually, I would say that the related bug is actually a dupe of this one...
especially since the same code will fix both problems..
*** Bug 88201 has been marked as a duplicate of this bug. ***
nav triage team:

Not a 1.0 stopper, pushing out to mozilla1.0.1
Target Milestone: mozilla1.0 → mozilla1.0.1
reassigning history bugs to new owner - send this bug back to me if it looks
like something I should fix (such as embedding-related architecture issues),
rather than the actual history owner...
Assignee: alecf → blakeross
Status: ASSIGNED → NEW
Target Milestone: mozilla1.0.1 → ---
Saw this was still in the release notes, and thought I might have an idea how to
fix it. Just a wild guess, but here goes.

I assume the current implementation of this tracks each time a HTML document is
loaded, and sets some variable (say, "$lastpage") to that (or maybe it directly
sets the preferance which would be a small perf problem fixed by my idea). This
would explain why framesets cause problems.

Instead of keeping track of it and seting $lastpage over and over and over again
during Navigators life, why not this:

When a Navigator is closed, check if any others are running 9windows or tabs).
If there aren't, grab whatever is in the URL bar and set
browser.history.last_page_visited to it. If the user quits, rather then closes,
grab the URL from the current window and set the pref to that.

Two issues:
1) This will end up seting browser.history.last_page_visited to that last window
you closed, not the last URL that loaded. I think this is an improvement myself,
but felt it should be noted. After all, that last window I closed was the last
page I was viewing, regardless of what order it may have loaded in.

2) What to do if there are multiple Nav's open, and a user quits in Mailnews?
I imagine the following would be possible: for each Nav open, grab the URL in
the URL bar, and ask Cache how stale it is. Most recent one gets saved to
browser.history.last_page_visited. This might have a few minor quirks in odd
situations where, say, site A and B are open, B was rendered most recently, but
before that Nav instance was at B, it reloaded A itself. A would be written to
the pref since the other window still referances it and causes the check against
cache staleness, but this assumes the user hit back or something to load site B,
and it didn't unstale it.

Anyway, just had that as an idea and figured I'd throw it out there. I'd be nice
to get rid of these bugs that are high profile enough to make the release notes.
Also handles IFRAMEs fine, not sure if the current implementation has issues
with that too.
Target Milestone: --- → Future
*** Bug 84615 has been marked as a duplicate of this bug. ***
Nope...this fix would also cause problems.

Consider a web site like about.com which opens links inside a subframe. Now you 
have a problem figuring out what to change the last page visited to: was it a 
frame or a real page? If we take it on the nav bar, this problem isn't fixed at 
all. Try this out by linking away from any topic at www.about.com, and you'll 
see that the nav bar does not change. Also, if we update the global history 
based only on pages loaded and not on frames, we still have a problem in 
deciding whether a page opened in a frame is considered a page or a frame. 
Needless to say, this can cause quite a headache.

I think I have a better solution than the above: why not keep track of the last 
link followed, including ones typed into the nav bar? It would be easier to set 
the pref with this because all that would have to be filtered out is the 
protocol (mail, news, http, ftp, etc).
I agree with Ryan Egesdahl comment #23.

The other advantage of this approach is that it would not count annoying auto
spam popups from pr0n that open as fast as you can shut them from becoming the
last site visited.

This can be a real problem as sometimes the only way to get rid of everything is
to turn off the browser, then you turn it back on and are back at square one!
i think if some frames are told to auto-refresh (like for ads) mozilla will load
that frame, because it was the last frame loaded. it should load the entire
frameset.

i'm curious, is this ever going to get fixed? because the bug makes the "last
page visited" option very annoying. it would be a very useful option if it
worked. now i open mozilla and all i see is an advertisement instead of the last
page i visited. who would want to use the option with this bug?
*** Bug 149532 has been marked as a duplicate of this bug. ***
*** Bug 166845 has been marked as a duplicate of this bug. ***
I found this bug today in Moz 1.2b. I wanted to note that I AM having the
problem with <IFRAME> tags.

1. In Prefs, select "Last page visited."
2. Go to http://www.perl.com/pub/a/2002/10/p6pdigest/20021016.html.
3. Exit the browser and restart. You get the ad in the page (which is in an
   iframe) instead of the page URL.
*** Bug 181204 has been marked as a duplicate of this bug. ***
*** Bug 184206 has been marked as a duplicate of this bug. ***
So would we be looking at patching things so frames aren't added at all to the
GlobalHistory or just that frames shouldn't be marked as the last page visited?
Would it be as simple as checking to see if the current page is a frame?
Attached patch First pass patch v1.0 (obsolete) — Splinter Review
This patch adds a boolean parameter to nsGlobalHistory's AddPage, if true then
the URL isn't marked as being the last page visited.
I would like to know what the general consesus is on whether the calls from
HidePage and MarkPageAsTyped should trigger the page as being last page visited
- in this patch they do not.
This patch will probably prevent embedded being compiled as some calls to
nsEmbedGlobalHistory's AddPage will need the extra parameter added.
Comment on attachment 125211 [details] [diff] [review]
First pass patch v1.0

This so needs work.  Don't ever change a frozen interface.
Attachment #125211 - Flags: review-
Thanks to biesi for the idea and help

This patch makes LastPageVisited attribute readwritable, renames
SaveLastPageVisited to SetLastPageVisited and moves the call to it from
nsGlobalHistory.cpp to nsDocShell.cpp.

With this patch applied mozilla now checks to see if the page is a frame or not
and if not checks to see whether it needs to be marked as the last page
visited.
Attachment #125211 - Attachment is obsolete: true
There appears to be two options on how to change the current behaviour:

1)We remember just the top level frameset, not the frame
2)We remember the page as a whole, including whatever frame was loaded last

Option 1 seems to be the easier patch to do and gives lastpagevisited consistent
behaviour as far as frames are concerned and would behave the same as bookmark
and homepage setting.

Option 2 would give a better solution to the end user but I suspect that needs
to be spun off into separate bug and hopefully can share code with
bookmark/homepage setting if that gets fixed for frames (bug 25909 I think).

The current patch, attachment 125257 [details] [diff] [review], implements option 1.
Attachment #125257 - Flags: review?(caillon)
Comment on attachment 125257 [details] [diff] [review]
Revised patch that doesn't change frozen interfaces

Requesting review from radha
Attachment #125257 - Flags: review?(caillon) → review?(radha)
Attachment #125257 - Flags: superreview?(alecf)
Comment on attachment 125257 [details] [diff] [review]
Revised patch that doesn't change frozen interfaces

interesting. kind of hacky, but it works. (the hackyness was already there with
the various prefs - this patch obviously doesn't make it any worse) 

sr=alecf
Attachment #125257 - Flags: superreview?(alecf) → superreview+
Comment on attachment 125257 [details] [diff] [review]
Revised patch that doesn't change frozen interfaces

Changing r request to bryner
Attachment #125257 - Flags: review?(radha) → review?(bryner)
+          NS_ENSURE_SUCCESS(browserHistory->SetLastPageVisited(spec.get()),
NS_ERROR_FAILURE);

I don't think that a failure to set the last visited page should be fatal...
Attachment #125257 - Flags: review?(bryner)
Attached patch Patch v1.2Splinter Review
Revised patch taking on board comment #39 and comments by timeless on IRC.
No longer returns a fatal error if page isn't set and succession of GetIntPref
is checked.
Also added remove of SaveLastPageVisited from header file.
Attachment #125257 - Attachment is obsolete: true
Comment on attachment 126025 [details] [diff] [review]
Patch v1.2

Requesting r and sr
Attachment #126025 - Flags: superreview?(alecf)
Attachment #126025 - Flags: review?(timeless)
Comment on attachment 126025 [details] [diff] [review]
Patch v1.2

sr=alecf
Attachment #126025 - Flags: superreview?(alecf) → superreview+
Attachment #126025 - Flags: review?(timeless) → review+
Fix checked into trunk by timeless - thanks
Assignee: blaker → ian
Would be nice to get this onto the branch but will probably miss the cut off for
1.4 final as it sits on the trunk to make sure there are no unforseen issues
with it.
Status: NEW → ASSIGNED
Flags: blocking1.4?
*** Bug 210396 has been marked as a duplicate of this bug. ***
Blocks: 210748
Comment on attachment 126025 [details] [diff] [review]
Patch v1.2

Requesting driver approval for landing on the branch.
This patch makes last page visited behave consistantly.
At the moment if you have a frameset with frames A, B and C, last page visited
remembers the last of those frames to load, so sometimes it's frame A,
sometimes frame B and other times it's frame C.
This patch makes last page visited act in the same way as bookmarking and it
just remembers the frameset loading not the frames.
Patch has been on the trunk for about 4 days now with no bugs logged and has
been tested on linux for about 10 days before that.
Attachment #126025 - Flags: approval1.4?
Comment on attachment 126025 [details] [diff] [review]
Patch v1.2

Seeking driver approval for check in to 1.4.x branch, has been sitting on the
trunk for about 10 days without any regressions showing.
Attachment #126025 - Flags: approval1.4? → approval1.4.x?
Moving blocking request from 1.4 to 1.4.x
Now been on trunk 16 days without any reported regressions
Flags: blocking1.4? → blocking1.4.x?
Wouldn't hold 1.4.1 for this fix but will still consider the patch. Setting to
blocking-
Flags: blocking1.4.x? → blocking1.4.x-
Has anyone made a patch and tested this on the 1.4.1 branch? If it looks like it
would apply cleanly and could be tested in the next day or so we would probably
approve this for 1.4.1.
The patch as it stands does apply cleanly to the lastest 1.4.x branch on linux.
I've applied it, rebuilt mozilla and tested that the behaviour wrt last page
visited is as expected.
Comment on attachment 126025 [details] [diff] [review]
Patch v1.2

a=mkaply for 1.4.1
Attachment #126025 - Flags: approval1.4.x? → approval1.4.x+
Please add fixed1.4.1 keyword when checked in.
Flags: blocking1.4.x- → blocking1.4.x+
Keywords: fixed1.4.1
Checked into both 1.4 branch and trunk -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
I want to make a comment on this.
This change does break at least galeon and epiphany.
What does it break?
sorry for the missing information. One cannot build current galeon and epiphany.
The build-error is:

../mozilla/.libs/libmozillaembed.al(GlobalHistory.lo)(.gnu.linkonce.d._ZTV16MozG
lobalHistory+0x3c):/usr/src/packages/BUILD/galeon-1.3.7/mozilla/GlobalHistory.cp
p:68: undefined reference to `MozGlobalHistory::SetLastPageVisited(char const*)'
../mozilla/.libs/libmozillaembed.al(GlobalHistory.lo)(.gnu.linkonce.d._ZTV16MozG
lobalHistory+0x74):/opt/mozilla/include/appcomps/nsIGlobalHistory.h:28:undefined
reference to `non-virtual thunk [nv:-4] to
MozGlobalHistory::SetLastPageVisited(char const*)'
collect2: ld returned 1 exit status
make[3]: *** [galeon-bin] Error 1
Did you check if this patch has made it into those products? Can you post up the
particular lines (and some context) in nsGlobalHistory.cpp that the build fails on?
*** Bug 218720 has been marked as a duplicate of this bug. ***
Blocks: 224532
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: