Closed Bug 58613 Opened 20 years ago Closed 18 years ago
"last page visited" cannot handle framed pages properly
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...
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...
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
Status: NEW → ASSIGNED
Component: History: Session → History: Global
*** 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.
*** 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.
*** 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?
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.
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)
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...
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
Comment on attachment 126025 [details] [diff] [review] Patch v1.2 sr=alecf
Attachment #126025 - Flags: superreview?(alecf) → superreview+
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
*** Bug 210396 has been marked as a duplicate of this bug. ***
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+
Checked into both 1.4 branch and trunk -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 18 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: *** [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. ***
You need to log in before you can comment on or make changes to this bug.