Closed Bug 154018 Opened 23 years ago Closed 22 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)

Product:
Core
Component:
Layout: Images, Video, and HTML Frames
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: paper, Assigned: john)

References

(
URL
)

Details

Attachments

(4 files, 1 obsolete file)

2 goats testcase (zip)
579 bytes, application/x-zip-compressed
Details
Force Flush after </iframe>
627 bytes, patch
Details | Diff | Splinter Review
Attachment 92010 + forced flush in OpenContainer() for <iframe>
1.72 KB, patch
Details | Diff | Splinter Review
Flush before InitialReflow
878 bytes, patch
Details | Diff | Splinter Review
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: 22 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.

Attachment

General

Creator:
Created:
Updated:
Size: