58613 - "last page visited" cannot handle framed pages properly

Status: RESOLVED FIXED

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

Description

[sairuh (rarely reading bugmail)]

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!

Comment 1

[sairuh (rarely reading bugmail)]

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...

Comment 2

[sairuh (rarely reading bugmail)]

works fine in 4.7x. also adding correctness.

Comment 3

[Eric Pollmann]

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. ?

Comment 4

[Blake Ross]

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

Comment 5

[Blake Ross]

oh.  that is where we set it, I think.

<slaps forehead>

Comment 6

[Blake Ross]

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.

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

when you wish upon a star...

Comment 8

[Matthew T (active 1999-2002)]

This probably depends on an as-yet-unreported bug which bug 25909 will also 
depend on.

Comment 9

[Viswanath Ramachandran]

nav triage: radha - can you please dup or depends this bug. 

Comment 10

[Radha on family leave (not reading bugmail)]

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. 

Comment 11

[Viswanath Ramachandran]

moving to mozilla0.9.1

Comment 12

[Paul Chen]

*** Bug 56134 has been marked as a duplicate of this bug. ***

Comment 13

[Paul Chen]

Marking nsbeta1+. Leaving mozilla1.0 milestone until I can figure out where to
schedule this.

Comment 14

[Alec Flett]

*** Bug 81378 has been marked as a duplicate of this bug. ***

Comment 15

[Alec Flett]

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..

Comment 16

[Jeremy M. Dolan]

Filed related bug 84615, regarding "last page visited" opening to internal
popups like the 'get plugin' window.

Comment 17

[Alec Flett]

actually, I would say that the related bug is actually a dupe of this one...
especially since the same code will fix both problems..

Comment 18

[sairuh (rarely reading bugmail)]

*** Bug 88201 has been marked as a duplicate of this bug. ***

Comment 19

[Paul Chen]

nav triage team:

Not a 1.0 stopper, pushing out to mozilla1.0.1

Comment 20

[Alec Flett]

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...

Comment 21

[Jeremy M. Dolan]

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.

Comment 22

[Blake Ross]

*** Bug 84615 has been marked as a duplicate of this bug. ***

Comment 23

[Ryan Egesdahl]

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).

Comment 24

[Robert Thorneycroft]

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!

Comment 25

[Jeff Tilak]

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?

Comment 26

[Boris Zbarsky [:bzbarsky]]

*** Bug 149532 has been marked as a duplicate of this bug. ***

Comment 27

[Boris Zbarsky [:bzbarsky]]

*** Bug 166845 has been marked as a duplicate of this bug. ***

Comment 28

[David DelGreco]

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.

Comment 29

[R.K.Aa.]

*** Bug 181204 has been marked as a duplicate of this bug. ***

Comment 30

[u60234]

*** Bug 184206 has been marked as a duplicate of this bug. ***

Comment 31

[Ian Neal]

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?

Comment 32

[Ian Neal]

Attachment: First pass patch v1.0

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 33

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 125211 [details] [diff] [review]
First pass patch v1.0

This so needs work.  Don't ever change a frozen interface.

Comment 34

[Ian Neal]

Attachment: Revised patch that doesn't change frozen interfaces

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.

Comment 35

[Ian Neal]

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 36

[Ian Neal]

Comment on attachment 125257 [details] [diff] [review]
Revised patch that doesn't change frozen interfaces

Requesting review from radha

Comment 37

[Alec Flett]

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

Comment 38

[Ian Neal]

Comment on attachment 125257 [details] [diff] [review]
Revised patch that doesn't change frozen interfaces

Changing r request to bryner

Comment 39

[Christian :Biesinger (don't email me, ping me on IRC)]

+          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...

Comment 40

[Ian Neal]

Attachment: Patch v1.2

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.

Comment 41

[Ian Neal]

Comment on attachment 126025 [details] [diff] [review]
Patch v1.2

Requesting r and sr

Comment 42

[Alec Flett]

Comment on attachment 126025 [details] [diff] [review]
Patch v1.2

sr=alecf

Comment 43

[Ian Neal]

Fix checked into trunk by timeless - thanks

Comment 44

[Ian Neal]

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.

Comment 45

[Ian Neal]

*** Bug 210396 has been marked as a duplicate of this bug. ***

Comment 46

[Ian Neal]

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.

Comment 47

[Ian Neal]

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.

Comment 48

[Ian Neal]

Moving blocking request from 1.4 to 1.4.x
Now been on trunk 16 days without any reported regressions

Comment 49

[Asa Dotzler [:asa]]

Wouldn't hold 1.4.1 for this fix but will still consider the patch. Setting to
blocking-

Comment 50

[Asa Dotzler [:asa]]

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.

Comment 51

[Ian Neal]

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 52

[Mike Kaply [:mkaply] (Out until 9/29)]

Comment on attachment 126025 [details] [diff] [review]
Patch v1.2

a=mkaply for 1.4.1

Comment 53

[Mike Kaply [:mkaply] (Out until 9/29)]

Please add fixed1.4.1 keyword when checked in.

Comment 54

[Ian Neal]

Checked into both 1.4 branch and trunk -> FIXED

Comment 55

[Wolfgang Rosenauer [:wolfiR]]

I want to make a comment on this.
This change does break at least galeon and epiphany.

Comment 56

[Ian Neal]

What does it break?

Comment 57

[Wolfgang Rosenauer [:wolfiR]]

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

Comment 58

[Ian Neal]

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?

Comment 59

[Bill Mason]

*** Bug 218720 has been marked as a duplicate of this bug. ***