97470 - iframes/frames should not display in history

Status: RESOLVED FIXED

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

Description

[Rafael Ebron (:rebron)]

Only the primary page should be in history, not content within a page, i.e. ads,
iframes.

Example, if I go to Netscape.com, I get ads.web.aol.com, gub.netscape.com,
toolbar.netscape.com all showing up in History.  None of those should show up in
History.

Comment 1

[Radha on family leave (not reading bugmail)]

I think that you mean these urls show up in the history window. That is global
history. 

Comment 2

[Jeremy M. Dolan]

Happens here too, Linux 0.9.3.

IFRAME (and FRAME, maybe...debateable) should not show in the History "manager"
thing (it doesn't have a unique name). Also, these pages are showing in location
bar history searching (not the dropdown). Only main pages should work there, as
well.

Comment 3

[Radha on family leave (not reading bugmail)]

This is global history and location bar autocomplete hooks up with global history.

Comment 4

[Peter Trudelle]

->blake

Comment 5

[Warner Young]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

Comment 7

[Rafael Ebron (:rebron)]

Todd, take a look at this bug and see if we can rank this higher in priority. 
It makes History suck a lot especially in the sidebar because the history
collects a lot of unecessary cruft like redirects, iframes, ads, making the
history list too large.

To replicate:
Clear history
Go to www.netscape.com notice the netscape.com folder
Go to www.cnn.com notice the cnn.com folder plus, an aol ad folder, and a folder
for the toolbar which is an iframe
Go to www.news.com notice a news.com folder plus a cnet.news.com folder

Total folders for three sites: 6

Here's the IE comparison:
Do the exact same sites
Total folder for the three sites: 3

Comment 8

[Todd Pringle]

Rafael, I agree that this is very suboptimal and was confusing to me at first as
well.  Alec Flett once told me that it was not trivial to fix and given the
other stuff that Blake has on his plate, I'm not sure this makes the cut. 
Blake, any idea how hard this is to fix?

Comment 9

[Blake Ross]

I'm going to cross my fingers and spread the love here...Alec will know how to 
do this much more readily than me, and he has already done preliminary work for 
hiding.

Comment 10

[Alec Flett]

*sigh*
I've seen related bugs.. this might be a dupe. However:
1) IFRAME contents should display in global history. an IFRAME is no different
than a normal FRAME, and we certainly want more to put FRAME loads into history.
I suspect that as more browsers support IFRAME, more and more sites are going to
start using it as an interactive way of displaying subdocuments like FRAMEs

the fact that some ads display there sucks, but there isn't a workaround that I
know of

2) 302-style redirects do not appear in global history (fixed that bug last
month) - and that is what is most often used for .. what other redirects are you
seeing in your history?

By the way, make sure you're using either a recent milestone build or Netscape
6.2.1 - one of the main reason we released 6.2.1 so we could hide 302 redirects....

I'm closing right now to get off the radar, but if you can come up with a
SPECIFIC type of redirect or a way of detecting that an IFRAME is an Ad

Comment 11

[Rafael Ebron (:rebron)]

Let me clarify from comment #2 re: frames and this debatable:
Desired experience is if I go to a framed site (index.html plus
framecontent1.html) history should display just index.html.

When I subsequently select a link in framecontent1.html that takes me to another
site within that frame i.e. framecontent2.html, framecontent2.html should
display in history.

What should not happen is when visiting index.html, framecontent1.html should
not show up in the history listing as that would be redundant.

IFRAMES should be treated the same way, visit to main url that has the iframe
should be the only url that's recorded in history.  When user visits another
page within that IFRAME space, that url should be recorded in history.

Alec, can you put in the bug # that this a dupe of.  Want to track the progress
of this feature.

Comment 12

[Jeremy M. Dolan]

> we certainly want more to put FRAME loads into history

Do we? I agree with Rafael, if I visit foo.com/frameset, I only want that in
history, not 50 sub-documents. Clicking the sub-documents in history is going to
bring up something differant from what I originally saw, since it's not with the
rest of the frames. That's no good. And gets in the way of clicking the main
frameset so I can use the site properly.

What this really calls for is multi-document pages to be tree'd in history:

[-] www.todd.com
       Todd's Front page (/)
   [-] Todd's model trains (/train-frameset/)
          Default view (loads default frame src='s)
          Pictures (loads pictures.html in left frame, other frames get default)
          Todd's Tips (loads tips.html in the left frame, other frames default)
       Todd's nunchuck collection (/whatever.html)

But until something like this can be done, I would much prefer my history not to
be spammed with hundreds of sub-documents.

Comment 13

[Alec Flett]

ok, there seems to be enough debate to warrant reopening this. I think you guys
have convinced me of the value here :)

Changes here probably involve docshell - we need to hide the page from the UI,
but store the URL in history so we can still color visited links. We can
probably leverage the hidePage() work from the 302 redirection fix.. however
we'll need a way to re-show the page in case anyone ever views it from the top
level.

I'm not sure where the dupe to this is though, if there is one.

unfortunately, I don't have time for this so I'll move to mozilla 1.1 for now

Comment 14

[Mark Renouf]

I'd like to point out that pages that aren't ever loaded end up in the history
as well. Type "http://foo.bar" into the url bar hit enter, click ok on the
error, then check the history sidebar. In fact, it's added to history before
resolving the hostname is even attempted.

Comment 15

[Alec Flett]

mark: that "feature" is new, and not related to this bug. Let's keep this bug
focused on frames.

Comment 16

[Rafael Ebron (:rebron)]

Nominating nsbeta1.  History usability would improve a lot with this fix,
allowing users to see the actual url that they visited rather than seeing the
url they visited plus all the iframes, plus all the ads, plus the redirects.

Comment 17

[Peter Trudelle]

nsbeta1+ per ADT triage team, to not include the ads and redirects.  Frame
elimination may not be so clear cut, and deserves a separate bug.

Comment 18

[Jaime Rodriguez, Jr.]

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

Comment 19

[Alec Flett]

I can't distinguish between ads and iframes and redirects are already fixed (as
discussed earlier in the bug), so I'm making this a frame-only bug... and
resetting the nsbeta1 nomination. Sorry for not updating the summary sooner.

Comment 20

[saari (gone)]

plussed to keep iframes in history, but not redirects and advertisements

Comment 21

[Alec Flett]

for the 2nd time, I'm un-plussing and asking that people read this bug more
carefully. Please see my previous comment. The latest "nsbeta1+" seems to
indicate that you're plussing this bug not being fixed (since if we fixed this
bug, we would NOT put iframes in history)

Comment 22

[Jesse Ruderman]

This really screws up autocomplete now that it sorts by frequency.
http://tinderbox.mozilla.org/SeaMonkey-Ports/panel.html is higher than
http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey and 
http://www.keepersoflists.org/cgi-bin/adverts/ads.pl?iframe;zone=homepage is
higher than
http://www.keepersoflists.org/.

The autocomplete problem could be fixed by leaving frames out of of
autocomplete, by not incrementing the visit count for frame loads, or by leaving
frames out of history entirely.

Comment 23

[Jesse Ruderman]

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

Comment 24

[Alec Flett]

ah good point. Actually, one other thing that I wanted to do with autocomplete
was to make "typed" urls take higher precident than any non-typed urls. Anyone
want to file a bug on that? that would also solve this problem.

Comment 25

[WD]

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

Comment 26

[WD]

Alec: Filed bug 141491

Comment 27

[Alec Flett]

mozilla 1.1alpha already passed, just moving out to 1.1beta

Comment 28

[Jesse Ruderman]

See also bug 94514, POST result page should not appear in global history.

Comment 29

[John Morrison]

I was playing around with some programs to page-sequence live content and for
interest I had a look at what the proportion of "real" child iframes and
frames is to "ad banner" iframes stored in history. Here's the numbers.

For a random set of 80 URLs (generated with a program that pumps the proper
nouns in /usr/dict/words into Google's "I feel lucky" form) there were nine
legitimate frameset pages (with 19 child frames added to history), _none_ that
used an iframe for something other than a banner advertisement, and 16 ad
banner iframes (also added to history). So that's 16 ad banners and 19 real
child frames added to history in the minimum case.

For a set of 30 media- and consumer-oriented homepages (e.g., netscape, aol,
espn, gamespot, etc.), (not including 3 popup ads), there were 4 real child
<frame>s, and _49_ iframe ad banner URLs added to the history. That means 60%
of the global history for those pages was 'ad banner' information. (Note: that
proportion will only increase over time, since you may revisit the same
toplevel content more than once, but the ad banner URLs will keep changing).

[Actually, that last number is somewhat of an argument for never storing
iframe URLs in global history, since their use on the web, for better or
worse, is almost always as an ad banner, and 'ad banner' URLs are never
"linked-to" from another page in practice (so link-coloring capability is 
moot). In fact, IE5.5/win2k will not "visit-color" a link to a URL (and 
apparently not store in history) until an anchor has been specifically clicked 
to load that URL. In other words, just loading a child <iframe> in a document 
does not count as part of IE's global history]. For example, try loading and 
reloading this page before clicking any of the links:

<html>
<body>
    <a     href="http://www.mozilla.org/"   target="net"> mozilla.org </a>
<br><iframe src="http://www.mozilla.org/"     name="moz"></iframe>
<hr><a     href="http://home.netscape.com/" target="map"> netscape.com </a>
<br><iframe src="http://home.netscape.com/"   name="net"></iframe>
<hr><a     href="http://www.mapquest.com/"  target="moz"> mapquest.com </a>
<br><iframe src="http://www.mapquest.com/"    name="map"></iframe>
</body>
</html>

Comment 30

[Jeremy M. Dolan]

I think /inline/ frames, by their very nature, are useless in history. Even if
they contained actually non-ad content, unless the user clicked the frame and
did 'show only' or 'open in new window', it's just a part of the parent document.

Are inline images added to history? Looking at my history db:

-rw-r--r--    1 mozilla   2894079 Jun 26 23:35 history.dat

It's almost 3 megs... that can't help startup or runtime perf, or bloat. My
history prefs say retain pages for 7 days. With the avg length of a URL, say, 50
bytes, have I actually visited 57,881 pages in the last 7 days? What's it
stashing in there? No wonder the history window takes 12 seconds to even
intially render here.

Comment 31

[John Morrison]

Uh-oh. I believe the size of that file is a different bug. See bug 112308.
We should, as alecf suggested, compress commit that file at shutdown. But
it was assumed that the situation of bug 112308 couldn't be reached by a 
"normal user". Apparently it can and I did notice at one point (mentioned
in email) that we seemed to be botching the logic for a compress commit and
letting that file grow without bound. Oy. 

jmd: comment in the other bug. If you can provide that file for analysis that 
would be great (although it is your history file so it's understandable if you 
don't want to share it :-). alecf: is there any tool that exists (or easily 
written) to dump stats-only and figure out just what is in that file 
(especially what is dead matter).

Comment 32

[Alec Flett]

moving bugs to mozilla 1.2alpha that missed 1.1beta

Comment 33

[Alec Flett]

Attachment: hide frames when they're added to global history

so this turned out to be pretty easy, and I was able to clean up some docshell
code in the process...

basically all I had to do is also call HidePage() whenever IsFrame() is true...
so instead of continuing to bloat up all the global history callsites, I
instead moved ShouldAddToGlobalHistory() into AddToGlobalHistory(), and added a
boolean "shouldhide" parameter to AddToGlobalHistory()

this also saves us the double-call to url.GetSpec() in the case of
hidden/redirected urls.

Finally, I removed the unused UpdateGlobalHistory() and changed the global
history methods to be non-virtual.

reviews?

Comment 34

[Radha on family leave (not reading bugmail)]

Alec, The changes related to Global history look fine. But where is the local
variable PRUint32 originalLoadType added to OnNewURI() used? 

Comment 35

[Alec Flett]

oops! that was part of some earlier debugging. I'll remove that variable right
now.. (I won't bother with another patch) other than that, its ready for a review.

Comment 36

[Alec Flett]

oh, and the reason I moved the equalUri over to the top of that if() is that
equalUri is almost always PR_FALSE, so the if() will short circuit sooner...

Comment 37

[Radha on family leave (not reading bugmail)]

Comment on attachment 96914 [details] [diff] [review]
hide frames when they're added to global history

r=radha  (with originalLoadType removed)

Comment 38

[Radha on family leave (not reading bugmail)]

Comment on attachment 96914 [details] [diff] [review]
hide frames when they're added to global history

r=radha  (with originalLoadType removed)

Comment 39

[John Morrison]

I realize that this bug is "...should not _display_ in history", but I was 
wondering if it shouldn't really be "... should not be added to history". 

I don't know if there is a requirement to note a frame/iframe visit (possibly 
link coloring but the IE behaviour for frames and iframes is to only color 
links to frames documents that have been previously loaded as a top-level 
document or loaded by the user directly clicking on a link to load the document 
in a frame (or iframe). Frames documents that are not "actively" loaded are not 
added to history).

My reason for concern is (as noted above) iframes are almost always ad banners, 
and they may constitute a substantial portion of all URLs ever loaded. And this
(hidden or not, unless I misunderstand the code) leads to a substantial amount 
of runtime bloat (a few hundred KB, I believe).

Comment 40

[Jeremy M. Dolan]

> if it shouldn't really be "... should not be added to history".

I was wondering the same. History bloat, and the obscene slowness of the history
window would benefit.

Comment 41

[Alec Flett]

Well, what I ran into with our current implementation is that the means by which
a page is loaded (i.e. LOAD_LINK vs. LOAD_NORMAL) is inherited from the parent
frame, so that if I click on a link (which gives you LOAD_LINK) that loads a
frameset, the toplevel frame and all its child frames are loaded as LOAD_LINK as
well. 

So I think what we really need to do, unfortunately, it provide even more
flags.. I'm concerned about further complicating docshell as it is, so I'm going
to land this and then lets open another bug about actually adding them, and
mention that THAT bug's purpose is to reduce bloat.

Comment 42

[Darin Fisher]

if you have access to the channel for a frame, you can determine whether or not
it is the top-level document.  check nsIHttpChannel::documentURI for this. 
cookies uses this to determine if a cookie is 3rd party or not, so there is some
likelihood that we actually set documentURI correctly ;-)

Comment 43

[Darin Fisher]

i should add that i agree w/ alecf... let's file another bug to handle the RFE
to not store URLs of frames in global history.

Comment 44

[Darin Fisher]

Comment on attachment 96914 [details] [diff] [review]
hide frames when they're added to global history

>+nsDocShell::AddToGlobalHistory(nsIURI * aURI, PRBool aHidden)

>+    ShouldAddToGlobalHistory(aURI, &updateHistory);
>+    if (!updateHistory) return NS_OK;

hmm... if we don't check the return value of ShouldAddToGlobalHistory, then
why return something?  maybe it could just return a boolean instead?!?

otherwise, looks good to me... sr=darin

Comment 45

[Alec Flett]

thanks darin.. 

Comment 46

[John Morrison]

Yeah, you're right. Land this and open a separate bug for the "don't store" 
point.

Comment 47

[Alec Flett]

thanks, fix is in.

Comment 48

[Jesse Ruderman]

I can verify that iframes no longer appear in autocomplete or in the history
window, and that links to the iframes are still marked as visited (on Windows XP).