Closed Bug 154018 Opened 19 years ago Closed 19 years ago

content in <iframe> displayed twice when <script> appears in outer page & in iframe page

Categories

(Core :: Layout: Images, Video, and HTML Frames, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: paper, Assigned: john)

References

()

Details

Attachments

(4 files, 1 obsolete file)

When a page has an <iframe>, followed by a <script src="..."> somewhere after
it, and the iframe page also has a <script src="...">, everything before the
<script src="..."> in the iframe will be duplicated.  Doesn't matter what the
scripts are.

testcase coming up.
Attached file 2 goats testcase (zip)
In this testcase, index.html has one iframe pointing to iframe.html.  It also
has one <script..> pointing to an empty jsOuter.js.  iframe.html has the word
"goat" in it only once, and a <script..> pointing to jsInner.js.

Both js files are blank.  This is not a requirement for the bug (they could
have code in them and the bug will still appear)

Expected Results: 1 "goat" in the iframe
Actual Results: 2 goats
For those who are lazy to unzip, here's a testcase URL
http://animecity.nu/mozilla/2goats

The second goat can not be highlighted.  Also, the DOM Inspector says the second
goat does not exist.  Somehow it's getting added to the document, but not really
getting added to it.. (?)
CCing people who might know what this is about
if the dom-inspector doesn't say it exists twice then my money is on that this
is a layout problem. Don't have a recent build atm so i can't try this out
What build are you using? The latest Windows nightly shows one "goat" in the iframe.
I had 20020620xx, and just downloaded the nightly:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.1a+) Gecko/20020625

Still seeing it.  Doesn't seem to matter if javascript is on or not, or if I'm
using cache.  I'll play with it the settings little more to see if I can get
only one goat.
I can not duplicate this bug on the 1.0.x branch -- only on the trunk.  I
created a new profile and I still see them.
Yeah, I see this on Linux current trunk too.
Status: UNCONFIRMED → NEW
Ever confirmed: true
the behaviour is very similar to that of bug 101148, although that bug doesn't
involve iframes
I have tracked this down to Bug 52334 (the massive iframe fixeup bug), so CCing
jst.  Since it's such a big patch, it's hard to pin down what's going on. 

This is basically what the parser is doing:
A) Script in both files
 1. >->HTML of index.html
 2.   >->BODY of index.html
 3.     >->IFRAME of index.html
 4.     <-<IFRAME of index.html
 5.     >->SCRIPT of index.html
 6.     <-<SCRIPT of index.html
 7. >+>HTML of iframe.html
 8.   >+>BODY of iframe.html
 9.     >+>Goat of iframe.html
10.     >+>SCRIPT of iframe.html
11.     <+<SCRIPT of iframe.html
12.   <-<BODY of index.html
13. <-<HTML of index.html
14.     <+<Any document.write from script loaded in iframe.html
15.   <+<BODY of iframe.html
16. <+<HTML of iframe.html

B) Script in iframe.html only
 1. >->HTML of index.html
 2.   >->BODY of index.html
 3.     >->IFRAME of index.html
 4.     <-<IFRAME of index.html
 5.   <-<BODY of index.html
 6. <-<HTML of index.html
 7. >+>HTML of iframe.html
 8.   >+>BODY of iframe.html
 9.     >+>Goat of iframe.html
10.     >+>SCRIPT of iframe.html
11.     <+<SCRIPT of iframe.html
12.   <+<BODY of iframe.html
13. <+<HTML of iframe.html

C) Script in index.html only
>->HTML of index.html
  >->BODY of index.html
    >->IFRAME of index.html
    <-<IFRAME of index.html
    >->SCRIPT of index.html
    <-<SCRIPT of index.html
>+>HTML of iframe.html
  >+>BODY of iframe.html
    >+>Goat of iframe.html
  <+<BODY of iframe.html
<+<HTML of iframe.html
  <-<BODY of index.html
<-<HTML of index.html

In otherwords, while index.html is waiting for it's script to load, iframe.html
begins to load.  iframe.html finds a <script> and pauses.  By that time,
index.html's script has loaded and index.html finishes up.  Finally, iframe.html
is allowed to finish up (after it has loaded it's script of course).

I think something funny is going on at A13.  The parser of course never sees the
2nd goat (nor does the DOM inspector)

I'll do some more digging (If anyone knowing where I should be looking, please
comment)
This works, but I need comments (review would be better ;) ) on whether I'm
ass-backwards.

The patch forces a flush upon closing an iframe container.  This results in the
iframe object being created.  Since it doesn't have the page loaded yet, we get
a blank iframe, thus preventing any chances that an iframe is created in
mid-load.
I'm searching deeper for a better patch.

What I am trying to hunt down is how to stop the goats in iframe from being
created twice.  Both goats get created in nsCSSFrameConstructor::ContentInserted,
the first one during the ConstructDocElementFrame call, and the second one
during the bm->ProcessAttachedQueue() call.  

Personally, I don't think the ConstructDocElementFrame should be creating stuff
when bm already has it in it's queue to be created (or does it at that point? I
don't know yet)

Can anyone with knowledge of nsCSSFrameConstructor::ContentInserted,
ConstructDocElementFrame, or bm->ProcessAttachedQueue() comment or theorize on
why the latter two functions are creating the same elements?
Sounds like something David was talking about that we did for <selects> and
which resulted in duplicated contents.
Could this be related to bug 153815?  It's a bad idea to call
ConstructDocElementFrame while you have ContentInserted notifications pending,
because you'll end up with duplication.  What I mentioned to Jonas yesterday was
bug 143862 comment 17.
dbaron: Yes, I'm running under the assumption that this bug and bug 153815 are 
the same.  Except this one has a smaller testcase and doesn't crash.

So we can force a flush after </iframes> (which is the current patch), or we 
could force a flush just before handling the script.  Both will create the 
iframe before it gets partially filled (and solve the bugs), but which is the 
best solution? or is neither the correct solution?
okay, I'm now convinced the Patch Attachement 89614 is the way to go.  Better to
force a flush on an </iframe> than a script, since there's less likely to be
pages with iframes on them.

Can I get a couple of people who are following this bug to r=/sr=?
If not I'll be forced to e-mail my request. ;)


If I remember correctly, the patch also fixes Bug 148827
Shouldn't the fix be not to call InitialReflow twice (which is what was causing
bug 153815)?
dbaron:
This testcase doesn't call InitialReflow twice for the same document. :)  

Bug 153815 does do InitialReflow twice, but I wanted a simpler testcase than the
one I created for it.. to rule out red herrings.  I think (but have no evidence)
that the second InitialReflow is just a side effect of the double insert.
Like bug 153815, this is a race condition between when the iframe gets
constructed by the outer-doc, and when the onStartRequest() handler for the
inner-doc is fired. The scenario for this particular bug is this:


1. The content sink for the outer-doc creates and inserts the iframe node
   into the document.

2. The act of inserting the iframe into the document (actually SetParent()) fires
   off a load request for the iframe's inner-doc.

3. The inner-doc's onStartRequest() callback is triggered and it goes about
   trying to create a document viewer, but since the outer-doc has not
   constructed a layout frame for the iframe, no widget exists for the new
   content viewer, so it bails on making a presShell and presContext. This means
   it will not call InitialReflow() for the inner-doc.

4. Some of the inner-doc's data is parsed, and the content sink creates and adds
   those nodes to the inner-doc's content tree, but the content sink does not
   call FlushTags() to force frame construction. In this specific example the
   textnode containing "goat" is added at this stage.

5. When the outer-doc gets around to constructing a layout frame for the iframe,
   a content viewer exists for the inner-doc so nsDocShell::SetVisibility()
   triggers DocumentViewerImpl::Show() which calls InitPresentationStuff() with
   a hard coded value of PR_TRUE, which tells it to do an InitialReflow(). This
   is the code that jst added/modified for bug 52334.

   This InitialReflow() call initiates frame construction for the document
   via a ContentInserted() call it makes. This has the side effect of creating
   frames for the document and any descendants it has, which in this case
   includes the "goat" text node.

   Normally this InitialReflow() is called as the result of the onStartRequest()
   callback being fired, so at that stage the document is normally empty.

6. It's important to know that the content sink keeps track of all the nodes
   it has created that have not yet been constructed with a call to FlushTags(),
   it assumes that it is responsible for calling ContentInserted() and
   ContentAppended() to trigger frame creation for those nodes, so at some point
   after the iframe's construction, the content sink for the inner-doc calls
   FlushTags() creating a 2nd text frame for the "goat" text node.


This is the reason the content looks doubled on screen. Note that the content
nodes are not doubled, just the frames created for each node that existed when
InitialReflow() was called for the inner-doc.

So now having said all this, I have a fix (attachment 92010 [details] [diff] [review]) for bug 153815
which prevents the InitialReflow() from happening in step 5 above. The content
sink assumes that it will be the only one calling InitialReflow() for the
document it is creating, so InitialReflow() should never be triggered from
DocumentViewerImpl::Show() while in the middle of loading a document.

But as ArronM (paper@animecity.nu) pointed out in bug 153815, the patch has the
side effect of modifying what happens with the test case in this bug (attachment
89003 [details]). That is, it makes the test case not render anything.

What's happening with my patch applied is this:


1. The content sink for the outer-doc creates and inserts the iframe node
   into the document.

2. The act of inserting the iframe into the document (actually SetParent()) fires
   off a load request for the iframe's inner-doc.

3. The inner-doc's onStartRequest() callback is triggered and it goes about
   trying to create a document viewer, but since the outer-doc has not
   constructed a layout frame for the iframe, no widget exists for the new
   document viewer, so it bails on making a presShell and presContext. This means
   it will not call InitialReflow() for the inner-doc.

4. Some of the inner-doc's data is parsed, and the content sink creates and adds
   those nodes to the inner-doc's content tree, but the content sink does not
   call FlushTags() to force frame construction. In this specific example the
   textnode containing "goat" is added at this stage.

5. When the outer-doc gets around to constructing a layout frame for the iframe,
   my fix for bug 153815 (attachment) prevents InitialReflow() from being
   triggered, so no frame construction for the inner-doc is initiated at this
   point either.

6. The content sink for the inner-doc calls FlushTags() to trigger frame creation
   for the nodes it has not flushed yet, but since InitialReflow() was never 
   called, no frame exists for the body node, so
   nsCSSFrameConstructor::ContentAppended() bails out early and does nothing:


    // Get the frame associated with the content
    nsIFrame* parentFrame = GetFrameFor(shell, aPresContext, aContainer);
    if (! parentFrame)
      return NS_OK;


So we end up with no frames getting constructed.
Aha!  So the underlying problem is, InitialReflow creates the frames and
FlushTags() does too.  Possible solutions:

1. Flush before we call InitialReflow on the chance that children of the frame
have not been flushed
2. Don't let InitialReflow cause frame creation (probably will cause problems)
3. Check for the primary frame of content before creating a new frame for it

#1 is our most likely candidate for now.
Attached patch Flush before InitialReflow (obsolete) — Splinter Review
This patch fixes this bug but not the other one Paper's patch fixes.  Perhaps
we're looking at two different problems after all.
I think jkeiser attached a patch for a different bug, so I'm not sure what his
solution is. Two different solutions come to mind, with 2 different levels of
effort:

1. With the knowledge that both the Parser/ContentSink and the File loading
   On*Request() notifications are processed on the main thread, the simplest
   way to assure that the iframe is constructed before the inner-doc's
   OnStartRequest() notification is fired, is to flush in
   SinkContext::OpenContainer() before we return to the event loop.

   Paper's patch (attachment 89614 [details] [diff] [review]) flushes in CloseContainer(), but my one 
   concern with that is that there could be content between the open <iframe>
   and close </iframe> tags that either force a flush, or stalls the parser long
   enough to trigger the flush timer in which case the iframe would be added to
   the document and it's load request fired off before CloseContainer() was
   called for the iframe.

   I'm not a parser expert so everything I'm saying is in theory so someone
   more familiar with the parser implementation can verify/deny that this
   situation could ever happen. I'm also not sure if it's even safe to flush
   during an OpenContainer() call.

2. The other approach that comes to mind would require a more significant
   effort and involves delaying the inner-doc load request from being fired
   off until *after* the flush happens, this may or may not require some
   re-writing of when we fire off inner-doc load requests when dynamically
   inserting iframe nodes into the document via JS, etc.

I also think that the patch for bug 153815 (attachment 92010 [details] [diff] [review]) still applies no
matter what approach is used above.
Depends on: streetmap.co.uk
Here are the 2 patches I'm running with in my source tree.
The patch I meant to attach before.  It is is essentially Paper's patch except
it flushes as late as humanly possible (flushes are bad).  Paper's patch is
valid, but the fact that Paper's patch and my patch fix a different set of
stuff reveals that we may have yet *another* timing problem.  Or it could be
that I was extraordinarily unlucky in my reloading and my patch doesn't even
fix this bug :)

Regarding your concerns about Paper's patch: it *is* possible for a flush to be
forced before the </iframe>.  This will cause the iframe to be created; other
content will then be created, perhaps not flushed, but then when </iframe>
happens Paper's code will be triggered and it *will* be flushed.

Kin, I think your patch in attachment 92010 [details] [diff] [review] could use a bool somewhere to
determine whether the InitialReflow that was *supposed* to happen on the frame,
happened.  If it did not we do InitialReflow there.  If it did we do not do
anything.  It would be something like if (!mLoading && !mInitialReflowOccurred)
...

NEW OPTION: I have been thinking about the two InitialReflows and there may be
a way to get rid of it; if we flush frames in in onStartRequest() the frame for
the iframe will be there and we can go on with the InitialReflow and friends so
that there is no need to do it a second time later.  I have not investigated
this solution enough to be sure it's viable.
Attachment #93247 - Attachment is obsolete: true
I just posted a patch in bug 153815 (attachment 97077 [details] [diff] [review]) which basically merges my
previously proposed patch (attachment 92010 [details] [diff] [review]) with jkeiser's proposed fix above
(attachment 93310 [details] [diff] [review]) and adds some checks to execute them only when necessary. I
also tried to add enough comments to explain the situations when our patches
will be triggered.

jkeiser, jst and I discussed what you mentioned above under "NEW OPTION" a while
back, and decided it might not work in a case where someone inserts a hidden
iframe into the doc and then immediately makes it visible via JS. That is, it's
possible for the OnStartRequest() to be fired off when it is still hidden, in
which case flushing will have done nothing.
Marking Fixed, probably due to 153815
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
Component: Layout: HTML Frames → Layout: Images
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.