237717 - [FIXr]loading iframe or object with html file inta a positioned div causes extra entries of current document in history

Status: RESOLVED FIXED

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

Description

[Foteos Macrides]

User-Agent:       Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.6) Gecko/20040113
Build Identifier: Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.6) Gecko/20040113

If an ifame element which has a text/html file as its src, or an object element
which has a type="text/html" file as its data, is loaded dynamically into a
positioned div (layer), the current document is reiterated inappropriately in
the history.  The consequence is that one must hit the back button multiple
times to actually get back to the previous document.

Reproducible: Always
Steps to Reproduce:
1.use createRange() ... appendChild() to load <iframe src="foo.html"
...></iframe> or <object data="foo.html" type="text/html"...></object> into a
div with position:absolute.
2.check the history list (down-arrow button beside the back button) and you will
see that every time you do 1. another entry of the current document is added
inappropriately to the list.
3.

Actual Results:  
Because of the inappropriate reiterations of the current document in the
history, one must hit the back button multiple times (or jump over those
reiterations in the dropdown with the history list) to actually get back to the
previous document.

Expected Results:  
Neither IE nor Opera have this bug. Their back buttons take you back properly to
the previous document.  So should Mozilla'a.

Comment 1

[Tuukka Tolvanen (sp3000)]

bug 148794 perhaps?

Comment 2

[Foteos Macrides]

When I searched for this bug with "iframe object history" as keywords I did find
148794 and some others which seemed related, but not quite the same.  Those
involved writing content into an iframe, and none mentioned object.  The bug I'm
reporting involves writing an iframe or object element into a positioned div. 
In any case, hold on to my URL to check when you folks think you might have
fixed the bug.

Comment 3

[Matthias Versen [:Matti]]

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

Consider the following scenario:

1)  You have an iframe on the page
2)  I click a link in the iframe, which loads a new URL in the iframe

Under those conditions, we need to add an entry to session history so that you
can go back.  This is the code that is getting triggered by that testcase (which
you should attach to the bug, by the way).

It's not quite clear to me how the two cases may easily be disambiguated; right
now I suspect that we treat any frame load that happens while the toplevel
document is not loading as a page traversal like above.

Comment 5

[Foteos Macrides]

No.  You appear to be mixing apples with oranges, and that appears to be the
kind of thing that's behind the jam Mozilla finds itself in with regard to this
matter.  The W3C DTDs define iframe and object as inline elements.  Their
content should NOT be associated with the session history for the current "full"
window, any more than should the content of other elements within the overall
document being displayed in that window.  The iframe and object with
type="text/html" do have a similarity to new windows like those created via
window.open(). Also, an iframe can be a target, like a bona fide frame. And so
it is understandable that one would instinctively expect an iframe or a object
with type="text/html" to have an accessible history, but if so, it should be
independent of the current window's session history.

The way IE handles the history issue for iframe (doesn't handle it for object
with type="text/html") is that if you follow links within the iframe and then
right-click WITHIN the iframe, the back and forward buttons of the resultant
menu apply to those links travered within the iframe.  But, again, one should
not muddle such an "sub-history" if one sets that up (it's not necessary based
on the DTDs, but also not precluded), with the session history for the current
"full" window's documents.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> You appear to be mixing apples with oranges

_I_ am merely describing what the code currently does so that whoever comes
along and tries to fix this bug will know where to look.

> but if so, it should be independent of the current window's session history

Why?  Why is an iframe any different from a frame in this regard?  From a user's
perspective it's not -- a user doesn't care whether you use a frame, iframe,
object, or some other method because they all look and act the same in the end.
 That's not changing.  The IE method was considered, but there was wide
agreement that it's incredibly painful and counterintuitive (and that there is
no strong motivation for it).

Note that this behavior is not incompatible with fixing this bug; it's just that
fixing this bug must not break this behavior.

Comment 7

[Foteos Macrides]

I am unclear about what you are referring to with the terms "the IE behavior"
and "this behavior".  In any case, I did some more exploring and here's what I
found.  IE, Opera and Mozilla all behave essentially the same when the iframe or
object with type="text/html" are encased in a p or an ordinary div.  They indeed
are treated "like a frame" -- with the session history listing only the URLs,
but keeping track of whether they should be placed in the iframe (or object)
versus the overall window as one presses the back and forward buttons.  However,
if the iframe or object with type="text/html" is encased in a positioned div
(layer), IE and Opera continue to treat them "like a frame" whereas Mozilla
starts reiterating the URL of the overall document in the session history
instead of the URLs being loaded into the iframe or object, and so the back and
forward buttons appear to be doing nothing as you cycle through those
reiterations of the same (and wrong) URL.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Partial fix (not quite right)

This fixes the original problem reported by not adding to session history when
we're doing the initial load in an iframe.

Really, though, this is wallpaper over a bug (in nsDocShell::CloneAndReplace,
probably).  With this patch, we don't store the right history state for the
iframe (since we're in fact not storing its history state at all), so appending
it to the document, following a link, then going back breaks.

What _should_ be happening is that when we append the iframe its history state
is attached to the current page history state (instead of creating a new one as
we do now).  Like the "replace" part of CloneAndReplace needs to work.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Slightly better

OK, so the old behavior is:

  When a subframe loads, grab the current history entry of the subframe, walk
to
  the root docshell, get the current toplevel history entry, clone it,
  recursively, except replace the current history entry of the subframe (when
we
  get to it) with the new entry we are loading.


This behavior makes perfect sense _except_ when the subframe has no current
history entry.	This is what happens when we have an initial load in a subframe
(as in this bug).  In this case, there is nothing to replace, so we end up
constructing an exact clone of the toplevel history entry, which is sorta bad.

The old behavior is:

  Just like the old behavior, except if the subframe has no current history
  entry, append its new history entry to the current history entry of its
  parent.

This is exactly what we would do if the parent were still loading (well, we'd
append to the loading history entry, but if we're done loading the current one
is the one to use), and to me it makes sense to treat the two cases the same
way.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 144103 [details] [diff] [review]
Slightly better

Adam, could you review?  I've done the following tests (and this patch passes):

1)  Load page with static iframe, click link in iframe, test that back works
correctly.

2)  Load page with dynamic iframe created via button.  Click the button.  Click
a link in the iframe.  Test that back works correctly (btw, without this patch
it doesn't in this case).

3)  The original testcase in this bug (the bogus history entries no longer
appear).

4)  As test 2, but the dynamically created iframe shows a page with a link to
another page as in test 2.  Click that link.  Then click the button in the
iframe, which creates an iframe within the iframe.  Follow a link in that
innermost iframe.  Click back -- this should go one page back in the innermost
iframe.  Click back again -- this should go one page back in the outer iframe.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Taking...

Comment 13

[Foteos Macrides]

Attachment: Reporter's testcase

Creating a "Reporter's testcase" attachment for the document at the Reporter's
URL to help ensure that the patch for this bug also is tested for objects with
type-"text/html" and not just iframes.

Comment 14

[Foteos Macrides]

Ugh. the links in the "Reporter's testcase" do not work because the base is
changed to http://bugzilla.mozilla.org/ and I didn't include a base tag to keep
it http://www.macridesweb.com/oltest/  Here is a URL for the document I intended
to be used:

http://www.macridesweb.com/oltest/Bug237717.html#iframecontent

including a fragment to get you to the relevant section of the document.  I also
edited the input field for the URL: above to this URL plus fragment, but I am
relatively new to Bugzilla and don't know if that will work.

The patch as described seems appropriate, but what about this scenario (which my
testcase was intended to help test):

You dynamically load an iframe or an object with data="foo.html" and
type="text/html" into a positioned div and make the div visible.  You follow
links within that iframe or object thus extending the session history.  While
the positioned div stays visible your back and forward buttons can load
appropriately.  However, at some point your DHTML restores the positioned div to
hidden.  You now have entries in the session history which can't be displayed. 
What happens when you proceed to those entries via the back or forward buttons,
or history list dropdown menu?

Comment 15

[Foteos Macrides]

Let me amplify that last question.  In Boris' testcase the "Add ifraame" and
"Remove iframe" buttons use createElement() ... appendChild() to add, and then
removeChild() to remove, an iframe element into the document's body, with
reformatting of the entire decument each time the respective buttons are hit,
and with the same foo.html docuement (the Mozilla home page) being used as the
iframe src each time the "Add iframe" button is used.  In my testcase, I lnstead
load iframe or object elements dynamically into a pre-defined positioned div
(with an id of "overDiv" and initially hidden) in conjunction with onmouseover
events for a number of different links.  I also always use the same foo.html
file as the src for the iframe or data for the objects in my testcase, but
normally the different links would use different HTML files.  Thus once the
positioned div is hidden again, one normally would not be interested in the
history for any links followed from within, and displayed within the iframe or
object.  Whenever the positioned div is dynamically loaded and made visible via
one or another event, the sub-history for that content (an iframe or object in
these cases) should again start from scratch, as one expects for a dynamically
loaded positioned div which normally gets HTML fragments (rather than a complete
HTML file via the trick of making an iframe or object the dynamically loaded
content).  That is why my initial orientation was not to treat the iframe or
object "like a frame" when it ls dynamically loaded into a positioned div such
that the sub-history survives beyond the point when the positioned div is again
hidden.

Here's another way to phrase the issue.  Should the handling (persistence) of
such "sub-menus" be different when the iframe or object with data="foo.html" and
type="text/html" is loaded dynamically into (is made the child of) a positioned
div versus the body element?  Furthermore, should it matter whether the src or
data value for an iframe or object currently being loaded into the positioned
div or body element is the same or a different HTML file from that which was
previously loaded and had accumulated a sub-history?

But for now, the patch for not adding to session history when doing the initial
load of an html file for the subframe, and then appending the subframe's history
to the parent's (if I'm interpreting Boris' comments correctly)should take care
of the most serious bug issue.
 

Comment 16

[Boris Zbarsky [:bzbarsky]]

> tested for objects with type-"text/html" and not just iframes.

Mozilla uses the same exact code for both, so testing one is sufficient,
actaully.  ;)

(In reply to comment #14)
> What happens when you proceed to those entries via the back or forward
> buttons, or history list dropdown menu?

From the user's point of view, nothing changes.  In my testcase, nothing
changes, in fact.  In your testcase, since the frame still exists, the document
loaded in the frame actually changes (though the user can't see that).

It's not completely clear what should happen here, if only because there are so
many ways to hide a frame that it's not really possible to determine whether one
is hidden... and since unhiding iframes is also possible, changing the state of
history depending on the hidden state of the frame could be very confusing to
the user, just like the "nothing happens" behavior.  We'd need to come up with a
decent description of what _should_ happen, methinks (and perhaps we should file
a separate bug on this issue).

(In reply to comment #15)
> Should the handling (persistence) of such "sub-menus" be different

No.  A lot of pages put _all_ their content in a positioned div, and we don't
want to break iframe history on those pages.

> should it matter whether the src or [....] is the same or a different HTML
> file

This should be treated as it normally would be in history.  That is, it depends
on the type of load being done (eg if the load is done with location.replace(),
there should be no new history entry, but if the load is done normally there
should be).

Comment 17

[Foteos Macrides]

Your reasoning seems sound, even though the patch will yield some seemingly
strange (but "explainable") history historesis for my DHTML popup library.  I
looked at other browsers in this context, and Opera 7.23 is behaving for both
iframe and object with data="foo.html" and type="text/html" as Mozilla will do
with your patch.  IE behaves that way only for iframe.  For object with
data="foo.html" and type="text/html" it actually has the current Mozilla bug,
i.e., when you activate links within the object, it clones the parent's URL in
the session history instead of appending the URL for the subframe link.  You can
see that most easily via the onmouseover popup for the "With object scrollbar,
caption and Close link" anchor in my testcase.

Comment 18

[Adam Lock]

Comment on attachment 144103 [details] [diff] [review]
Slightly better

r=adamlock

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 144103 [details] [diff] [review]
Slightly better

darin, could you sr?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Checekd in for 1.8a.

Comment 21

[Jungshik Shin]

Would it be possible to include this in 1.7branch? Apparently, this fix fixed a
'back-button issue' in bug 235398

Comment 22

[Boris Zbarsky [:bzbarsky]]

Session history code is notoriously fragile; changes to it are very dangerous
and often lead to regressions in bizarre circumstances.

As a result, I'm absolutely opposed to landing this on any branches until it's
baked on the trunk for a good long time (preferably through an alpha milestone
release and ensuing bug reporting); even then I would be wary of doing so on a
long-lived branch unless some exhaustive regression testing has been done (I'm
talking along the lines of testing every single resolved session history bug in
bugzilla to make sure it didn't regress).

In addition to those concerns, see bug 235398 comment 9.

Comment 23

[Boris Zbarsky [:bzbarsky]]

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

Comment 24

[Boris Zbarsky [:bzbarsky]]

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

Comment 25

[Boris Zbarsky [:bzbarsky]]

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

Comment 26

[Jungshik Shin]

(In reply to comment #22)

> As a result, I'm absolutely opposed to landing this on any branches until it's
> baked on the trunk for a good long time (preferably through an alpha milestone
> release and ensuing bug reporting); 

  It seems like this criterion is now satisfied (of course, it is not if there's
a regression reported during 1.8alpha-cycle). 

> even then I would be wary of doing so on a
> long-lived branch unless some exhaustive regression testing has been done (I'm
> talking along the lines of testing every single resolved session history 
> bug in bugzilla to make sure it didn't regress).

  This is certainly not. I'll try to do this.

Comment 27

[Boris Zbarsky [:bzbarsky]]

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

Comment 28

[Jungshik Shin]

I completely forgot about this bug because I've been using 1.7.x with
libdocshell.so built with the patch applied. As a result, ff 1.0 was released
with this problem, which is rather unfortunate for Korean users (one of top
sites is rendered very hard to navigate due to this bug). I wanted to put up a
'hot fix' for them (i.e. docshell so/dll/dylib), but  firefox distribution has
that dll/so/dylib along with many other dll/so/dylib's folded into the main
firefox binary (firefox-bin on linux) making it impossible(?)/hard to replace
only docshell. 

I'm wondering if any session history regression has been reported  since the
patch went into the trunk.If not, one of two bz's conditions for branch landing
would be met.   

Comment 29

[Boris Zbarsky [:bzbarsky]]

I'm not actually aware of any regressions that have been reported since this was
checked in.  I'm not sure how far that carries given the ridiculously low amount
of testing trunk has been getting (major and obvious regression issues have been
taking weeks to report...)

Comment 30

[Boris Zbarsky [:bzbarsky]]

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