Closed Bug 148794 Opened 21 years ago Closed 4 years ago

document.write into iframe adds entries into session history

Categories

(Core :: DOM: Navigation, defect)

Product:
Core
Component:
DOM: Navigation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.4alpha

People

(Reporter: megabyte, Unassigned)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: topembed+)

Attachments

(3 files, 1 obsolete file)

Testcase
278 bytes, text/html
Details
More refined patch
3.07 KB, patch
adamlock
: review+
alecf
: superreview+
Details | Diff | Splinter Review
Screenshot - IE 6 on testcase
18.49 KB, image/gif
Details 
If document.write is used to change the content of iframes, an entry is added to
the session history each time it is used.  This has several adverse affects,
including not allowing the browser to go back to the real previous page, and
making it infeasible to use dynamically generated iframes as mouseover popups
since each mouseover would generate one or more history entries.
2002060208-trunk
Attached file Testcase — 
Though it isn't the main issue, to reproduce the broken back behavior, click
the link, shift-reload, click the link again.  The back button can no longer be
used.
Keywords: mozilla1.1
See also bug 148772 and bug 35340.
Session history bug.  Already filed, I believe...
Assignee: jst → radha
Component: DOM Level 0 → History: Session
QA Contact: desale → claudius
Whiteboard: DUPEME
Target Milestone: --- → Future
*** Bug 140565 has been marked as a duplicate of this bug. ***
Wrong #4: THIS should be duplicate of 140565 !
We have also a case in Netscape.com which the contents of the iframe is
affecting the session history:

http://developer.netscape.com/evangelism/sidebar/
Keywords: topembed
*** Bug 161501 has been marked as a duplicate of this bug. ***
Additional Comments!  This bug and bug #140565 is no longer a problem with Moz
BuildID# 2002080711.  You can read bug #161492 for more information since
someone mentioned that this bug #161492 is reproductable every time.
Actually, comment #1 is fixed, but the main part of the bug still remains.
What are the main part of the bug that still remain??  Can you provide some info
to shed some light on this one.  
Exactly what I said in my original report.  For example, use my testcase.  If
you click that link, you will have to hit back twice (or as many times as you
clicked the link + 1) to get to the previous page.
Cool!  It work pretty well on mine!  Never had a problem so far!  Try the latest
nightly build by download it fresh from the mozilla.org.  The nightly build from
a few day before doesn't work.
I'm using 2002080718.
Platform/OS -> All (since duplicates occur on other platforms)
OS: Windows XP → All
Hardware: PC → All
Added URL taken from bug 106995.
URL: http://bugzilla.mozilla.org/attachmen...
Keywords: mozilla1.2
This should be broken into several distinct bugs
Keywords: topembed → topembed+
Like what?  I only see one bug: the one described in the very first sentence of
my original report.
Whiteboard: DUPEME
"Though it isn't the main issue, to reproduce the broken back behavior, click
the link, shift-reload, click the link again.  The back button can no longer be
used."
Read comment 9.
oops, sorry. Nevermind then!
This is a little complex, as iframes could be something that are hidden and show
up only on mouseover, mouse click etc.... OR it could be something that is
present statically on a page (like regular frames) and pages are loaded on them
through another event like onLoad or even used pretty much like a regular frame.
  In the former case, we do not want the url loaded in them to be added to
session history, but in the latter case, we do want them  to go into session
history so that back and forward on them will show exactly what the user had
when they left the page.  

For the former case,  we have to identify the transient stage of the iframe and
docshell should be passed appropriate loadFlags so that urls loaded in them will
not go into session history. In the latter case, the iframe should be treated
like a regular frame and therefore pages loaded there should be added to session
history.  

More on identifying iframes to come .....


The following segment of code identifies  a IFrame in nsDocShell.cpp

  //identify the frame that hosts it.
    nsCOMPtr<nsIDOMElement> frame_element;
    nsCOMPtr<nsPIDOMWindow>
win_private(do_GetInterface(NS_STATIC_CAST(nsIDocShell *, this)));
    NS_ENSURE_TRUE(win_private, NS_ERROR_UNEXPECTED);

    win_private->GetFrameElementInternal(getter_AddRefs(frame_element));
    nsCOMPtr<nsIDOMHTMLIFrameElement>  iframe(do_QueryInterface(frame_element));

A non-null value for iframe indicates that the container is a IFrame. 
Any news on this one ? 1.2 is on track, so keyword will be obsolete soon.
Status: NEW → ASSIGNED
Target Milestone: Future → mozilla1.3beta
Target Milestone: mozilla1.3beta → mozilla1.4alpha
*** Bug 192996 has been marked as a duplicate of this bug. ***
Attachment #114472 - Flags: review?(adamlock)
Comment on attachment 114472 [details] [diff] [review]
patch v1.0

r=adamlock
Attachment #114472 - Flags: review?(adamlock) → review+
Attachment #114472 - Flags: superreview?(alecf)
Comment on attachment 114472 [details] [diff] [review]
patch v1.0

can we contain this check in a msDocShell::IsIFrame() method? I ask mostly
because we already have a nsDocShell::IsFrame() and it would be consistent...

Also, why not cast "this" to nsIInterfaceRequestor when you pass it to
do_GetInterface.. that way if there is an specialized template for
do_GetInterface, it will avoid the extra QI()
(there may not be one now, but I see a use for it right here!)
Attachment #114472 - Flags: superreview?(alecf) → superreview-
Good Point. Didn't notice that we had a IsFrame(). Now there is a IsIFrame()
and it is used in couple of places.
Attachment #114472 - Attachment is obsolete: true
Attachment #114697 - Flags: review?(adamlock)
Comment on attachment 114697 [details] [diff] [review]
More refined patch

r=adamlock
Attachment #114697 - Flags: review?(adamlock) → review+
Attachment #114697 - Flags: superreview?(alecf)
Comment on attachment 114697 [details] [diff] [review]
More refined patch

much cleaner! 
sr=alecf
Attachment #114697 - Flags: superreview?(alecf) → superreview+
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
What about the 1.3final branch ? It's still time to check in, 3 bugs still open.
So... what happened to the issues raised in comment 21 exactly?  This patch
broke normal iframe navigation.  See bug 199471.

beanladen@arcor.de, that's why we do NOT take risky changes in the last days of
a milestone....
I decided not to keep any loads to iframes in history. I know this could break
few web sites, but keeping iframe loads  in history causes more trouble than not
keeping 'em. 
It looks like this one should be reopened and blocking 1.4b+ 
due to bug 199471.
Requesting REOPEN and blocking 1.4b? due to bug #199471 .
Flags: blocking1.4?
I think the non-fix behaviour was acceptable compared to what we have now,
e.g., enter the guided bug reporting form, search for your key words, then 
look into one of the bugs you get, and then type back... Not a very nice 
experience for a bug reporter...
Patch 114697 has been backed out because of regression bug 199471.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: blocking1.4? → blocking1.4-
is bug 208835 related to or a duplicate of this bug?  The date that this patch
was backed out corresponds pretty closely to my findings in bug 208835 (builds
on 5/23 work as expected where builds on 5/25 don't work as expected).

Jake
*** Bug 208835 has been marked as a duplicate of this bug. ***
So could someone _clearly_ explain what problem this bug covers at this point? 
And preferably attach a testcase demonstrating the problem, since comment 9
claims the testcase in coment 1 is no longer relevant?
The initial report and testcase still apply.
1. Open testcase
2. Click Rewrite Frame
3. Click Back
What happens: Stay on same page, but return to state before Rewrite Frame was
clicked
What should happen: Go back to previous page
> What happens: Stay on same page, but return to state before Rewrite Frame was
> clicked
> What should happen: Go back to previous page

Why?  Why is this fundamentally different from setting the source of the iframe,
exactly?  In my opinion, what we are doing now is in fact correct....
IE and Opera behaves exactly like Mozilla on this issue.
Boris, please see my comments in bug 208835.  Specifically, try the following
link...
http://www.ashleyit.com/rs/jsrs/select/php/select.php

As per my instructions in said bug, make sure to close all browser windows, open
a up a brand new window, and go the jsrs link.  Choose various car Make's and
Models.  Check out the history being generated in the back button.  Visiting
these history entries usually does nothing, but I've seen at times where it
shows the contents of the hidden iframe which is definitely not expected (saw
this in 0.8 version of Mozilla Firefox, but not sure if I can guarantee it to be
easily reproduceable).

None of this stuff happened until the fix for this bug was backed out.  On
5/23/2003, no history was generated for the hidden Iframe.  On 5/25/2003,
history was being generated as is the behavior today.  I am not absolutely
certain this bug is the cause, but it seems pretty clearly related.

BTW, try this out in IE.  No back button history is generated.  I don't know
about Opera.  I don't use it much.

Jake
Comment 44 is incorrect.  IE (6.0.3079) and Opera (6.0.1010) both perform as
"What should happen."
Again, the biggest annoyance is dynamically changing ads in iframes.  For each
time it changes while you're on a page, you have to hit back + 1 in order to go
back to the real previous page.
It's also an annoyance on pages that use iframes to do server-push of dynamic
content. For example, http://www.ironplanet.com/j/equip/auction.jsp?groupId=206
-- try it in IE and the back button works as expected, but Mozilla doesn't want
to go back to where you came from.
(In reply to comment #47)
> It's also an annoyance on pages that use iframes to do server-push 

Server push is bug 211715, not this bug.

(In reply to comment #45)
> Boris, please see my comments in bug 208835.

Bug 208835 was incorrectly dupped to this bug, as far as I can see.  The issue
there is that there is an "invisible" iframe that gets modified each submit,
along with whatever else is going on, right?  That's not at all the same as the
testcase in this bug.  That bug should be reopened and a minimal testcase (one
not involving hundreds of lines of JS) created.  Then I'll see what I can do as
far as fixing it.

So.  Now back to this bug.  I don't have IE, and I'm not happy with the fact
that people are claiming different results for this bug's testcase in IE.   What
does IE actually do on the testcase for this bug?

(In reply to comment #46)
> Again, the biggest annoyance is dynamically changing ads in iframes.

I notice you very carefully didn't answer my question.  If a page changes the
contents of an iframe via setting src, should the history change?  Why or why
not?  Make sure to read bug 199471 before answering.  Is the situation with
document.write different from setting .src?  Why or why not?
Nice testcase is the Tinderbox
http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey
Click on a name, select one entry in the popup of checkins,
after that go back to the tinderbox. History now shows two
entries, but hitting the back button does not bring you back.
The frame showing the checkins menu should not have generated
any history entry. Instead, it gives a bogus one which blocks
the button.
The first fix which was backed out seemed to have removed 
also important entries, see my comment #37 on the bugzilla
bug report form which got incorrectly cleaned after going back.
I think (at least for the tinderbox type popup iframes), no
history entry should be generated. Maybe there are different
usages which should generate an entry (the bugzilla form ?).
> Nice testcase is the Tinderbox
> http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey

Time to update some builds.  That was bug 237717, not this bug.  It's been fixed
for a week and a half (trunk only; that patch is likely not going on the
branch).  I totally agree that that case should not have generated a history
entry (that's why it's been fixed, see).

So just to make that clear, doing any sort of testing with session history and
frames with a branch build or a trunk build older than April 13 is pretty
pointless at this point.
(In reply to comment #48)
> So.  Now back to this bug.  I don't have IE, and I'm not happy with the fact
> that people are claiming different results for this bug's testcase in IE.  
What
> does IE actually do on the testcase for this bug?

Maybe it was totally unrelevant to point out how IE behaves on the testcase
(since this is bugzilla and not IEzilla), but since I know did it and I was
told to be wrong, I add this screenshot to show how the latest IE 6 with all
the patches act. IE 6 start adding entries to the session history after the
second click, not like Mozilla that does it already on the first one.
I cannot reproduce this bug on Opera 7.23 nor 7.5B1 (was wrong before).
> Maybe it was totally unrelevant to point out how IE behaves on the testcase

No, it's very relevant.  Since there is no spec on what should be done here, we
should do "something reasonable"; what other browsers do will influence what's
reasonable.

I'm a little surprised myself that we add an entry on the first click, what with
the fix for bug 237717.  If that's the only issue people have I can try look
into it...
Boris (comment #50):
Blocking button is not fixed in 1.7rc1, and the iframe still causes an 
additional history entry in the tinderbox example. Weird: It seemed to
be fixed for the first opened window, but if you open a second window
and try the same thing, the button is again blocked after going back.
Tested with 1.7rc1 gcc 2.95.3 compiled Linux i686.
Additional info to #53:
It works (with an additional history entry) on the 1.7 branch tinderbox, but
not on the seamonkey main tinderbox (still the blocking problem). It does
NOT depend on the window (I loaded the seamonkey in the second window instead
of the 1.7 tinderbox ...).

(In reply to comment #53)
> Boris (comment #50):
> Blocking button is not fixed in 1.7rc1

Please actually read comment 50.  Keep reading it until you actually notice the
"not on the branch" part.  Please stop testing this bug or any other
frames-and-history bugs with 1.7anything builds.  It's a waste of time.
(In reply to comment #52)
> I'm a little surprised myself that we add an entry on the first click, what
> with the fix for bug 237717.  If that's the only issue people have I can try
> look into it...

Note that Opera 7.23 and the recent Opera 7.50 behave like the Geckos, i.e.,
history entries start being made with the first click in the testcase, instead
of starting with the second click as for IE.

I don't understand Boris' comments in relation to the fix for Bug 237717.  The
initial load into the iframe for the testcase is with an src, that happens to be
"about:blank" and thus yields no visible iframe content. It does not yield a
history entry, just as with the fix for Bug 237717 you do not get a history
entry no matter what the src value is for the initial load. The first click in
the testcase invokes a document.write that does create visible iframe content,
but to me that seems homologous to using an href for an actual document (with
content like that of the document.write) and with the iframe as the target. That
does, and it seems to me should, yield a history entry.  If you add additional
links to the testcase which perform document.writes with different content, you
can use the back button to cycle back through the various contents in the
iframe, just as you would expect if the links had hrefs for actual documents
with the iframe as the target.  So it seems to me that the way it is now makes
the most sense, albeit that there are no explicit standards in this case and it
is not what IE is doing.

The real issue is what's indicated in Comment 21.  One might not want the same
behavior when the iframe is a static component of the document versus when it is
the content of a positioned div that flips between being hidden and visible in
relation to events (such as onmouseover and onmouseout).  We discussed this in
conjunction with Bug 237717 and the decision there was, in effect, that trying
to deal with static versus DHTML iframes differently could yeild more problems
than solutions.

And to help drive home another important point that might not yet be clear to
everyone, the fix for Bug 237717 is in the trunk, which will eventually yield
the Mozilla 1.8 milestone, not in the present branch heading toward the 1.7
milestone, so get a nightly build (if you don't have one since Gecko/20040412)
to exercise the code with the fix for Bug 237717.
> It does not yield a history entry

Actually, it DOES.  That's why when you write into the iframe you can go "back"
to the about:blank page.  That's the weird part -- about:blank pages shouldn't
be generating history entries, if I read nsDocShell::ShouldAddToSessionHistory
correctly.
(In reply to comment #57)
> > It does not yield a history entry
> 
> Actually, it DOES.

Perhaps I'm being dense, or I don't understand what you are referring to by
"it", but it appears to me using Mozilla 1.8a2 (Gecko/20040522) that it does NOT.

That is, when I click on the "Testcase" anchor in the Bug 148794 document, which
has an href attribute with the value
"http://bugzilla.mozilla.org/attachment.cgi?id=86055&action=view", I get a
document that begins with an anchor that has the content "Rewrite Iframe" and is
followed by a static iframe that has an src attribute with the value
"about:blank".  The history has an entry for the bug report document, as it
should, and not for the "Testcase" document.  So the iframe with
src="about:blank" did NOT generate a history entry. Only the "Testcase" document
generated a history entry for the bug report document, as it should.  If I hit
the Back button at that point, I go back to the bug report document, not stay
inappropriately in the "Testcase" document.  This is exactly the same as what
happens if the iframe has src="blank.html" and the blank.html document is blank
between its body start and end tags.  One actually does that when using iframes
with SSL servers, because otherwise IE will stupidly issue its "non-secure"
warning for src="about:blank".

Now, if I click on the "Rewrite IFrame" anchor, its href attribute value is a
javacript URL which does a document.write into the iframe.  That DOES generate a
history entry, for the "Testcase" document with src="about:blank" in the iframe.
       This is the same as what would happen if the anchor had a URL pointing to
a physical document instead of content generated via a javascript URL, and IMHO
is the correct thing to do in either case.  So now, if you hit the Back button,
you go "back" to the "Testcase" document with src="about:blank" in the iframe. 
But this is NOT because the initial load with src="abaout:blank" generated a
history entry.

Everything is working as one would expect, based on what was decided during the
discussion of Bug 237717, and nothing "strange" is happening.

But it is not necessarily what those of us who use iframes in conjunction with
DHTML might initially imagine that we want, as reflected in Aaron's initial
Description of Bug 148793, until we think about the "complexity" referred to in
Radha's Comment 21, and in the discussion of Bug 237717.

> Perhaps I'm being dense, or I don't understand what you are referring to by
> "it", but it appears to me using Mozilla 1.8a2 (Gecko/20040522) that it does
> NOT.

We're talking about the same "it".  But we're talking about different things
when we use the words "history entry".  You're talking about entries in the
history dropdown thingy in the UI, which are really rather incidental from my
point of view.  I'm talking about session history entry data structures
associated with the docshell tree.  They should not be created for about:blank
frames.  That's why the fix for bug 237717 works -- it detects the situation
when we are doing a URI load in a docshell that doesn't already have a session
history entry associated to it and doesn't treat the load as creating a new
session history entry tree (these trees are what the UI shows as "session
history entries").

I just reread the code again and it looks like what's going on is that in the
case here we do have an mOSHE for the about:blank page (unlike the situation in
bug 237717 where the frame is created complete with the right src, in this case
the about:blank load completes before the new URI is set).  So that explains the
difference in behavior...
(In reply to comment #59)
> We're talking about the same "it".  But we're talking about different things
> when we use the words "history entry".

I see.  So if you modify the code such that src="about:blank" does not start a
sub-history for the iframe, the Geckos will behave more like IE. Note that IE
also does not start a sub-history for src="" but does for src="blank.html" where
blank.html is a physical file that is blank between the body start and end tags.

Once one or more javascript URLs have done a document.write into the iframe and
a sub-history has been created, with IE the Back and Forward buttons encompass
only the loads with that content in the iframe, and you no longer can access the
document with the iframe having src="about:blank" or src="" via those buttons. 
I presume this will be another consequence for the Geckos as well.

(In reply to comment #52)
> I'm a little surprised myself that we add an entry on the first click,
> what with the fix for bug 237717.  If that's the only issue people have
> I can try look into it...

Boris,

I thought consensus had been reached that you indeed should try to prevent the 
mOSHE for an about:blank page (i.e., if and iframe's src or object's data 
attribute does not indicate a file, plus no document.write has been done for 
the iframe or object).  Did you encounter a problem, or just not yet find the 
time for this?

I just haven't had time to deal with this, and given the number of other frame
session history bugs it looks like we just need a very different approach to
this subframe mess...
Same problem with page

http://quatramaran.ens.fr/~monniaux/wikilinks/pagea.html

and Mozilla 1.4 and 1.6.

which loads "tooltip" popups using IFRAMES (with src=...) triggered by
Javascript. For every tooltip shown, the page is entered into history; this
breaks the "back" button.
> and Mozilla 1.4 and 1.6.

David, that's not this bug.  That's bug 237717, which is fixed in current
builds.  As  you would have known had you read this bug before commenting on it...
*** Bug 321084 has been marked as a duplicate of this bug. ***
Blocks: backtraps
Component: History: Session → Document Navigation
QA Contact: claudius → docshell
Assignee: radha → nobody
I think this causes https://support.mozilla.org/no/questions/975037 (but there are some nested IFRAMEs involved, so it's hard to observe where the navigation really takes place.. In any case, I vote for following Chrome and Opera and not create history entries for back button navigation purposes when document.write() creates a new document inside an IFRAME.
On csmonitor.com, one has to click back twice most of the time when returning from articles. This part of Google's ad delivery code, called from a method called googletag.display(), special-cases Firefox and works around the issue - I'm still looking for the exact code that doesn't:
 
				h = b;
				try {
					g = a.contentWindow ? a.contentWindow.document : a.contentDocument, -1 != navigator.userAgent.indexOf("Firefox") && g.open("text/html", "replace"), g.write(h), g.close()
				}
				catch (Qd) {
					jd("Could not write content into iframe using  the DOM standards method:" + Qd.message)
				}

(It's hard to debug because it's semi-random, yet frequent - probably depends on the ads served)

While it's good to see there is a workaround, it's still a nuisance we should kill.
Status: REOPENED → NEW
(Wired.com is currently pretty broken on mobile - one of the issues is that using back button in an article is pretty much impossible due to Mobify.js using document.write() - perhaps 'replace' should be the default mode for both explicit and implicit document.open() calls? Otherwise, we might also hack things so that fastback is *always* applied when going back to a document that was replaced by a doc.write() one - on the assumption that the risk of running the doc.write() call again will be lower..)
This bug seems like a pretty big area where Chrome works and we don't.

Is there anything we can do to get it on someone's radar?

Do we know what Chrome does differently internally?
In general our document.open/write follows the current HTML spec and Chrome doesn't.
But there are plans to change document.open/write in the spec, once it is clear what should replace the current stuff there.
> In general our document.open/write follows the current HTML spec and Chrome doesn't.
But there are plans to change document.open/write in the spec, once it is clear what should replace the current stuff there.

Wouldn't it make more sense to do what Chrome does for now so that we don't look broke? And fix to the spec once the spec is updated?
> Wouldn't it make more sense to do what Chrome does for now

Figuring out exactly what that is and whether it makes sense is exactly what the spec work is about.  

That said, the spec end is more or less sorted, subject to implementor feedback.  See bug 1489308 and the links therein.
Depends on: 1489308

Fixed by bug 1489308. document.open no longer adds session history entries.

Status: NEW → RESOLVED
Closed: 21 years ago → 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.