Closed Bug 254144 Opened 20 years ago Closed 10 years ago

Iframe reloads when moved around the DOM tree.

Categories

(Core :: DOM: Core & HTML, defect)

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

Tracking

()

Status:
RESOLVED INVALID

People

(Reporter: doug.copi, Unassigned)

References

Details

(Keywords: testcase)

Attachments

(6 files, 5 obsolete files)

testcase which correctly demonstrates this bug
499 bytes, text/html
Details
Demonstration of this bug using frame elements.
341 bytes, text/html
Details
simple testcase that manipulates DOM inside iframe
401 bytes, text/html
Details
Final Patch
7.47 KB, patch
smaug
: review-
Details | Diff | Splinter Review
mochitest for this bug
2.46 KB, text/html
Details
TC 254144.htm (one of the permutations, still asserts the behaviour this bug wanted to implement is correct)
837 bytes, text/html
Details
User-Agent: Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.0.1) Gecko/20020823 Netscape/7.0 Build Identifier: Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.0.1) Gecko/20020823 Netscape/7.0 Whenever an iframe node is pulled from the DOM tree, the contentWindow is set to null, forcing a reload of the iframe's content when it is placed back into the DOM tree. The content of the iframe is reloaded even if the iframe is placed back into the same location it was pulled from. Reproducible: Always Steps to Reproduce: 1. Go to http://home.earthlink.net/~dcopi/bug/swap_iframes.html. You will see Google load in the top iframe and Yahoo in the bottom iframe. 2. Type a string into the Google search text box inside the top iframe. (Don't press the "Google Search" button) 3. Click the link labeled "Swap iframes". Actual Results: The string that was typed into the google search box was lost because the content of the iframes that were switched are reloded. Expected Results: The content of the iframes should remain unchanged and the string that I typed into the search text box should have persisted during the moveing of the iframe nodes. This incorrect behavior can be observed any time an iframe node is pulled from the DOM tree and re-inserted, whether it is done via an appendChild (see URL example), a removeChild and then appendChild, or a replaceChild.
Please attach the testcase to this bug. One core problem here is that windows/docshells don't do so well at being removed from the window/docshell tree... Another one is that moving the iframe in the DOM can change its src (which should arguably reload it).
Attached file testcase which demonstrates this bug (obsolete) —
Original testcase contained invalid URL's
Attachment #155275 - Attachment is obsolete: true
I see this on LInux 2004102606
Keywords: testcase
OS: Windows 2000 → All
Yes as pointed out appendChild in the spec does a remove of the node from the tree first as the node can only have one parentNode... now the case is, is this correct behaviuor or is what we're reading as remove mean something else other that removeNode.. I would say it does... it simply means remove the node but not with a removeNode as this destroys the object... we just do a remove of the node as indicated so that we dont have a clone of the node attached in the original poisition.. when we do a removeNode as mozilla does it of course destroys the object then the appendChild creates it from the original objects attributes.. (incorrect).. the spec that I've read unless im reading the wrong spec just says "remove node first" NOT removeChild.
> it simply means remove the node but not with a removeNode as this destroys the > object removeChild doesn't destroy the object, in general. I'm not sure what gave you the idea that it does. And yes, the removal should be done with removeChild, with all attendant DOM events firing, etc, etc. Either that, or done with a function that is an exact copy of removeChild, which is the same result.
If the iframe is moved from a document with a different xml:base then the src changes. Should this cause a reload? What if the user has navigated to another page inside the iframe?
> If the iframe is moved from a document with a different xml:base then the src > changes. Should this cause a reload? I would say no.
Although that's debatable -- for an image we would reload. It's not clear how the behavior should interact with user navigation, yes. Something to think about once there is a topic of discussion (that is, once iframes don't get blown away on removal from the tree).
Confirmed in Firefox 1.5 RC1 on windows. very easy to reproduce
Blocks: stirdom
Confirmed with Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060126 Firefox/1.6a1
Status: UNCONFIRMED → NEW
Ever confirmed: true
Hardware: PC → All
No longer blocks: stirdom
The key difference lies between removing an IFRAME from the DOM (and then adding it back), versus simply moving an IFRAME within the DOM. If it is removed, then adding it back in of course should reload it in its initial state. If it is just moved to a different position in the DOM, it should retain its current state, including history and navigation. The problem though, if I am not mistaken, is that there is no way to move an IFRAME without removing it, as both appendChild and insertBefore inherently function on existing nodes by removing them first and then adding them elsewhere afterwards.
> If it is removed, then adding it back in of course should reload it in its > initial state. Why?
(In reply to comment #14) > Why? Actually yes this is a good point. Even if it was removed first, the only way it could be added back in later is if a reference to it still existed. And if this is so, then it should be added in its last current state.
The top "google" frame will be moved below the "yahoo" frame using appendChild 10 seconds after the page is loaded. Any text entered into the google search box will be lost during the move. tested with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1
*** Bug 343235 has been marked as a duplicate of this bug. ***
Blocks: 276824
*** Bug 357700 has been marked as a duplicate of this bug. ***
Blocks: 361853
I can confirm that this happens ON 3.0a8pre on Windows. An iframe reload is triggered even on the simplest DOM operations such as <div styleid="div1"> <iframe id="iframe" src="http://www.bbc.com"> </iframe> </div> <div id="div2"> </div> ... document.getElementById("div2").appendChild(document.getElementById("iframe"));
What is the timeframe on a fix for this? We don't need any more 7 year old bugs out there...
With designmode on the same problem occurs which means the contents are lost. This makes moving around a iframe used as a text box for user input very difficult.
Sorry, cant edit my reply. Also, the reference to the frame in window.frames is inexplicably removed. var iframe = document.getElementsByTagName('iframe')[0]; alert(window.frames[iframe.name]); //Correct. [Object Window] reference. document.body.appendChild(iframe); alert(iframe.name); //Correct alert(window.frames[iframe.name]); //null
The window.frames issue is a separate bug (perhaps bug 170799).
These symptoms suggest relation with bug 302616 and suggest general DOM manipulation issues interfering with iFrame. Although related bug seems to be improved in recent Firefox 3 builds, no improvements seem to be achieved regarding these test cases (all three test cases were probed using the two versions stated bellow). Versions: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12) Gecko/20080201 Firefox/2.0.0.12 FireShot/0.32 Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008022604 Minefield/3.0b4pre
Attached patch Patch proposal V1 (obsolete) — — Splinter Review
Here is the start of a patch to fix this bug. This is not meant to be a final solution to the problem. However, it does fix both test cases for whatever that's worth! jst & bz could you gentlemen please enlighten me on what nsGenericHTMLFrameElement::UnbindFromTree() needs to do instead of calling mFrameLoader->Destroy()? I suspect there must be something we need to do besides nothing. Also BindToTree() only loads the frame if the loader is null which might not work in all cases. Mike M.
Assignee: general → MikeM
Status: NEW → ASSIGNED
Attachment #306569 - Flags: review?(djst)
I think you meant jst@mozilla.org, not djst@mozilla.com. :) You can use ":jst" to match him.
Comment on attachment 306569 [details] [diff] [review] Patch proposal V1 Sorry. Wrong email in reviewer.. trying again.
Attachment #306569 - Flags: review?(djst) → review?(jst)
As I said in e-mail, at least the following need to happen: 1) The docshell needs to be removed from the docshell tree 2) All the places that expect all docshells to be in a docshell tree (esp. the security-related ones) need to be fixed. Most likely you also need: 3) Fix the leaks this causes through reference count cycles through the docshell (which does not participate in cycle collection).
Comment on attachment 306569 [details] [diff] [review] Patch proposal V1 r- based on bz's above comment.
Attachment #306569 - Flags: review?(jst) → review-
Do you have any ideas on how I can validate the integrity of the doc shell tree after I remove/re-add the frame from it? Preferably programatically... >Fix the leaks this causes through reference count cycles through the docshell Ideas on how to do this?
To be honest, if I knew how to do those three things offhand, I'd just write the code. The hard part of fixing this bug are those three things.
(In reply to comment #31) > >Fix the leaks this causes through reference count cycles through the > docshell > Ideas on how to do this? Two things mostly related to finding said leaks: first, run Mochitests. You can do this inside $OBJDIR/_tests/testing/mochitest with the following command: > python runtests.py --autorun --close-when-done --leak-threshold=0 When the tests finish running you'll get an error message if anything leaks. (You'll need a debug build to do this, but I assume you're already running with a debug build.) Currently I have us leaking only an nsTimerImpl and an nsThread in a full test run, so stuff beyond that, particularly if it contains docshells or anything they might conceivably use or entrain, could easily be something a patch of yours might cause. You may find the patch in bug 420154 useful in getting better leak reporting information, at least until it gets in the tree -- hopefully that's soon. :-) Second, write Mochitests that explicitly test iframe moving, loading, reloading, etc. in the normal places and also in "weird" cases, like say doing so from window and iframe onload handlers, mutation events, in the middle of synchronous execution of script within that iframe, etc. Try passing around references to iframes and their content windows across windows and across and page loads, as well as after the window containing each iframe is closed. We have lots of documentation on exactly how Mochitests should be written[1], and if you fire up Mochitests without --autorun and/or --close-when-done you can load any individual test and view its source to see how it's written. (You can also load arbitrary web pages or files in the Mochitest browser to check for leaks in non-Mochitest pages.) Finally, if you have any other questions regarding Mochitests I'm more than willing to do my best to answer them.
Thanks for that helpful info Jeff. That is what I was looking for! I've got a more complete patch I'm testing now but the option --leak-threshold=0 doesn't appear to be a valid option. Ideas?
Also if I run without the --leak-threshold switch I get the following error: -------------------------------------------- failed to get nsJSRuntimeService! nsStringStats => mAllocCount: 1127 => mReallocCount: 0 => mFreeCount: 1055 -- LEAKED 72 !!! => mShareCount: 1793 => mAdoptCount: 153 Server pid: 2292 Timed out while waiting for server statup. ------------------------- Why is it showing leaks and it hasn't even started yet? FYI I didn't build using a specific $OBJDIR. Objects are built in-line with sources. Not ideal but works better for me right now since re-builds are not picking up file changes unless I manually delete the .obj file for any file I changed. Last question: How can I build faster when I only want to re-build 1 or 2 static libraries and re-link firefox? make -f client.mk build is what I'm doing now. The debug cycles are unbelievably slow.
Are you sure you're using runtests.py? The Perl version that I'm currently trying to terminate with extreme prejudice doesn't support the argument. For a no-objdir build, I *think* you want python $topsrcdir/_tests/testing/mochitest/runtests.py. If you want to rebuild only parts, it requires some manual trickery; I'm not sure exactly what that trickery is, but generally building (just "make -C content" or similar) in the top-level directory containing the files you've modified (and browser/app if you're on a Mac) can be enough.
If you change content, you have to at least build in content/ and layout/build/. Depending on your build options, you may also need to relink libxul or whatnot...
(In reply to comment #36) > Are you sure you're using runtests.py? Yes. Look inside runtests.py The word "leak" does not appear in any of the options for this script. > The Perl version that I'm currently > trying to terminate with extreme prejudice doesn't support the argument. What exactly are you trying to say? > For a no-objdir build, I *think* you want python > $topsrcdir/_tests/testing/mochitest/runtests.py. Yes that is what I'm running and in that directory too. Here's the exact command I'm running in MINGW32: python runtests.py --autorun --close-when-done --leak-threshold=0 After it spits out the proper usage junk it says: runtests.py: error: no such option: --leak-threshold Without the leak option you get the error in comment #35. This is more concerning to me. I'm stuck! Why doesn't static building work with mochitest?
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/testing/mochitest/runtests.py.in&rev=1.16&mark=159-165#154 Maybe you need to update your tree? (As for the Perl thing, I thought it might be that you stumbled on instructions saying to use the Perl version -- it *is* the official one, for now, until I can get the Python one to fully replace it.)
All files were originally taken from firefox-3.0b3-source.tar.bz2 with a date of 2/26/2008. My wincvs has this tag next to the file: FIREFOX_3_0b3_RELEASE The runtests.py.in file is at version 1.2. Quite a bit older than your 1.16 in the link above! I assume the firefox 3 branch is not up to date with regards to tests? Can you fix this?
Realistically, you probably need to be hacking on the trunk code, what you get with |cvs co mozilla/client.mk; cd mozilla; make -f client.mk|. It's entirely possible that's the version of the file that was around for b3; it hasn't been around that long.
Ok I'll update CVS from HEAD. Honestly, why have a Firefox 3 branch if everybody works off the head?
(In reply to comment #42) > Ok I'll update CVS from HEAD. Honestly, why have a Firefox 3 branch if > everybody works off the head? We don't have a Firefox 3 branch (yet)... Why do you think there's a branch?
Isn't that what FIREFOX_3_0b3_RELEASE is? That's what CVS said after pulling down the tar ball firefox-3.0b3-source.tar.bz2.
(In reply to comment #44) > Isn't that what FIREFOX_3_0b3_RELEASE is? No, that's a release tag, which was made when Firefox 3.0 Beta 3 was built. HEAD (trunk) is where Firefox 3 development is actively taking place.
Thanks for explaining the difference. The wincvs doesn't really distinquish the two. Either that or I'm to stupid to understand it. >No, that's a release tag, which was made when Firefox 3.0 Beta 3 was built. HEAD (trunk) is where Firefox 3 development is actively taking place. Regardless shouldn't the tar ball I pulled down have everything in it? I shouldn't need to go to the HEAD to make unit tests work. Correct?
(In reply to comment #46) > >No, that's a release tag, which was made when Firefox 3.0 Beta 3 was built. > >HEAD (trunk) is where Firefox 3 development is actively taking place. > Regardless shouldn't the tar ball I pulled down have everything in it? I > shouldn't need to go to the HEAD to make unit tests work. Correct? The tarball has everything in it from when the tarball was made, as it's a one time thing. Many changes to the unit tests have been made since then, so the tarball is very much out of date, and therefore, if you expect to use new features, you'll need to use the HEAD.
Sorry I thought I had pulled down a nightly... Guess that wasn't the case! Sorry for wasting your time fellas. I'm many hours into pulling down the HEAD from cvs already. None of this explains the "failed to get nsJSRuntimeService" error I received in comment #35 when trying to run tests, however. I suspect that problem will still exist when I finally get done re-building again.
simple testcase that manipulates DOM inside iframe + aref
Attachment #209807 - Attachment is obsolete: true
Attached patch Patch proposal V2 (obsolete) — — Splinter Review
This version fixes the bug, passes all mochitests without adding leaks of any kind. I'm looking for input on the // TODO comments left in the patch as well as more suitable names to replace two methods added by me AddToDocShellTree() & RemoveFromDocShellTree(). There is currently 1 known issue. I'm not sure what the correct behavior should be. It involves hitting refresh on the main browser window. Open testcase labeled "simple testcase that manipulates DOM inside iframe V2" and click the link. It should load google into the IFrame. Now refresh the browser. Should the IFrame contain Google or the original "foobar" text? If this is wrong the issue stems around the fact that GetChildSHEntry() for childOffset zero is returning something different after loading google into the IFrame. I don't understand this mechanism well enough to know what is wrong or how to fix it. I attempted to as you will see in the patch but fell short. I doubt its hard to fix but I need help. Please CC others who have expertise in Frames/DocShell for review and testing. If you are more qualified than me to finish this feel free to take the bug. Thanks!
Attachment #306569 - Attachment is obsolete: true
Attachment #307391 - Flags: review?(jst)
Attachment #307391 - Flags: review?(jst) → review?(Olli.Pettay)
Comment on attachment 307391 [details] [diff] [review] Patch proposal V2 r- based on IRC discussion. (Needs lots of testing)
Attachment #307391 - Flags: review?(Olli.Pettay) → review-
The refresh issue I mentioned in #50 is a separate bug that exists with and without my patch. Bug# 421507 has been filed.
I believe this may be somehow related with a rather old (but still unconfirmed) issue: bug 302616.
When you say "this" what do you mean?
I read bug 302616. Theres no moving of DOM nodes going on. I see no relation sorry.
Removed V2 for simple test case now that bug #421507 has been filed. This patch has no affect on that bug.
Attachment #307387 - Attachment is obsolete: true
Attached patch Final Patch — — Splinter Review
Final patch. Mochitests coming next...
Attachment #307391 - Attachment is obsolete: true
Attachment #308016 - Flags: review?(Olli.Pettay)
I don't see anything in that patch addressing item 2 from comment 29. Or item 3, for that matter...
Attached file mochitest for this bug —
Heres the mochitest file. This belongs under the mozilla/content/html/content/test when checked in. Test was verified to contain 2 failures without the patch and sucess with my patch. This test will verify that unload() is NOT called when a child iframe is removed and re-added to the DOM. At the same time it verifies that scripts running in the child frame continue to run and are not stopped. This behavior is the same as in IE 6 & 7.
> I don't see anything in that patch addressing item 2 from comment 29. Or item > 3, for that matter... I see zero leaks in mochitest with --leak-threshold=0 is that is what you are referring to. In regards to item #2 from comment #29. Show me something that is broke and I'll fix it. Right now I see nothing. If you know this code that well (and it sounds like you do) produce a test case that breaks the patch and upload it here. I'll be happy to fix anything you find ASAP. Asking me to vaguely "go find something to fix which might be broken" is asking too much. Isn't that what mochitesting is for? This breaks nothing that I know of. Maybe you know better. Show me...
> I see zero leaks in mochitest That just means that nothing is testing the scenarios that would leak. For example, have document A with subdocument B. Set the frame element that contains B as a property on some node in B. Then remove that element from document A and forget about it. As far as I can tell, document B will never get garbage collected. > Show me something that is broke and I'll fix it. Any usage of mParentDocument, GetParentDocument is likely broken. Anything that traverses the docshell tree is likely broken. Anything that looks at root treeitems (of same type or not) is likely broken. To tell whether it's actually broken requires testing of those codepaths (nonexistent right now) and then code analysis. > produce a test case that breaks the patch It would be faster to write the whole patch plus tests myself, to be honest. See below. > Asking me to vaguely "go find something to fix which might be broken" is > asking too much. <shrug>. That's the hard part of fixing this bug: finding all the places that depend on the invariant you're changing and fixing them. You've done the easy part: changing the invariant. > Isn't that what mochitesting is for? Yes, if it had 100% coverage. Its coverage in this area is closer to 1%, I'd estimate.
Put another way, passing the existing tests and the minimal tests that directly test for the bug as originally filed is a necessary but not sufficient condition for a patch being correct. It must also pass tests that attempt to break the changed code in various ways. In an ideal world, such tests would exist. But in reality, they don't, and they need to be written.
> That just means that nothing is testing the scenarios that would leak. For > example, have document A with subdocument B. Set the frame element that > contains B as a property on some node in B. Then remove that element from > document A and forget about it. As far as I can tell, document B will never > get garbage collected. Do you have suggestions on how to solve this circular reference problem? I know this is pretty common on the JS side of things in the GC world. > Any usage of mParentDocument, GetParentDocument is likely broken. Anything > that traverses the docshell tree is likely broken. Anything that looks at root > treeitems (of same type or not) is likely broken. To tell whether it's > actually broken requires testing of those codepaths (nonexistent right now) and > then code analysis. I see no breakage as of yet with moderate code coverage. Not done yet either... Any specific example you could provide would help. I have one minor annoying issue that I'm working on. When RemoveItem is called immediately after setting the src property to the frame (which causes reload) the reload is completed after the item is removed (as expected) The issue is that the cursor is still an hourglass since the docshell's mParentWidget member is nulled out by the RemoveItem() see stack below: ------------------------- >docshell.dll!nsDocShell::SetParentWidget(nsIWidget * aParentWidget=0x00000000) Line 3794 C++ gklayout.dll!DocumentViewerImpl::Hide() Line 1999 C++ docshell.dll!nsDocShell::SetVisibility(int aVisibility=0) Line 3907 C++ gklayout.dll!nsSubDocumentFrame::Destroy() Line 773 C++ gklayout.dll!nsBlockFrame::DoRemoveFrame(nsIFrame * aDeletedFrame=0x0a79b5a4, int aDestroyFrames=1, int aRemoveOnlyFluidContinuations=0) Line 5419 C++ gklayout.dll!nsBlockFrame::RemoveFrame(nsIAtom * aListName=0x00000000, nsIFrame * aOldFrame=0x0a79b5a4) Line 5023 + 0x10 bytes C++ gklayout.dll!nsFrameManager::RemoveFrame(nsIFrame * aParentFrame=0x0b5ed6c4, nsIAtom * aListName=0x00000000, nsIFrame * aOldFrame=0x0a79b5a4) Line 695 C++ gklayout.dll!nsCSSFrameConstructor::ContentRemoved(nsIContent * aContainer=0x03e3ba10, nsIContent * aChild=0x0bdad150, int aIndexInContainer=11, int * aDidReconstruct=0x0012d868) Line 9649 + 0x12 bytes C++ gklayout.dll!PresShell::ContentRemoved(nsIDocument * aDocument=0x0c18b218, nsIContent * aContainer=0x03e3ba10, nsIContent * aChild=0x0bdad150, int aIndexInContainer=11) Line 4742 C++ gklayout.dll!nsNodeUtils::ContentRemoved(nsINode * aContainer=0x03e3ba10, nsIContent * aChild=0x0bdad150, int aIndexInContainer=11) Line 167 + 0xba bytes C++ gklayout.dll!nsGenericElement::doRemoveChildAt(unsigned int aIndex=11, int aNotify=1, nsIContent * aKid=0x0bdad150, nsIContent * aParent=0x03e3ba10, nsIDocument * aDocument=0x0c18b218, nsAttrAndChildArray & aChildArray={...}) Line 2842 + 0x11 bytes C++ gklayout.dll!nsGenericElement::RemoveChildAt(unsigned int aIndex=11, int aNotify=1) Line 2775 + 0x2a bytes C++ gklayout.dll!nsGenericElement::doRemoveChild(nsIDOMNode * aOldChild=0x0bdad174, nsIContent * aParent=0x03e3ba10, nsIDocument * aDocument=0x0c18b218, nsIDOMNode * * aReturn=0x0012db5c) Line 3425 + 0x13 bytes C++ gklayout.dll!nsGenericElement::RemoveChild(nsIDOMNode * aOldChild=0x0bdad174, nsIDOMNode * * aReturn=0x0012db5c) Line 2991 + 0x1a bytes C++ gklayout.dll!nsSVGUseElement::RemoveChild(nsIDOMNode * oldChild=0x0bdad174, nsIDOMNode * * _retval=0x0012db5c) Line 91 + 0x14 bytes C++ xpcom_core.dll!NS_InvokeByIndex_P(nsISupports * that=0x00000011, unsigned int methodIndex=2, unsigned int paramCount=1235788, nsXPTCVariant * params=0x00000000) Line 102 C++ ----------------- Finally when the document is done loading we hit this code which takes care of restoring the cursor. Stack to reach this code is below: nsCOMPtr<nsIWidget> mainWidget; GetMainWidget(getter_AddRefs(mainWidget)); if (mainWidget) { mainWidget->SetCursor(eCursor_standard); } -------------------------------------- >docshell.dll!nsDocShell::OnStateChange(nsIWebProgress * aProgress=0x03c90b04, nsIRequest * aRequest=0x0c0e8a9c, unsigned int aStateFlags=786448, unsigned int aStatus=0) Line 4920 C++ docshell.dll!nsDocLoader::FireOnStateChange(nsIWebProgress * aProgress=0x03c90b04, nsIRequest * aRequest=0x0c0e8a9c, int aStateFlags=786448, unsigned int aStatus=0) Line 1236 C++ docshell.dll!nsDocLoader::doStopDocumentLoad(nsIRequest * request=0x0c0e8a9c, unsigned int aStatus=0) Line 870 C++ docshell.dll!nsDocLoader::DocLoaderIsEmpty() Line 765 C++ docshell.dll!nsDocLoader::OnStopRequest(nsIRequest * aRequest=0x082161a8, nsISupports * aCtxt=0x00000000, unsigned int aStatus=0) Line 682 C++ necko.dll!nsLoadGroup::RemoveRequest(nsIRequest * request=0x082161a8, nsISupports * ctxt=0x00000000, unsigned int aStatus=0) Line 688 + 0x25 bytes C++ gklayout.dll!nsDocument::DoUnblockOnload() Line 5645 C++ gklayout.dll!nsDocument::UnblockOnload(int aFireSync=1) Line 5593 C++ gklayout.dll!nsDocument::DispatchContentLoadedEvents() Line 2846 C++ gklayout.dll!nsRunnableMethod<nsDocument>::Run() Line 262 C++ xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x0012f888) Line 511 C++ xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x0094c650, int mayWait=1) Line 227 + 0x16 bytes C++ ------------------------------ Can you suggest another way of getting a pointer to the nsGenericHTMLElement or its base class so I can turn off the hourglass cursor (and spinning icon)when the load is complete? I could simply cache another temporary pointer in docShell which is set whenever RemoveFromDocShellTree() is called and nulled out whenever AddToDocShellTree() is called so that I can always access the parent. This would mean adding a member to DocShell like: nsCOMPtr<nsIWidget> mParentWidgetWhenDetached; Any pictoral documentation regarding the object model would be nice too have too...
> Do you have suggestions on how to solve this circular reference problem? Not offhand, sorry. I just realized that one other thing to double-check is that the widget and view (and viewmanager) are being properly reparented. In fact, it might be a good idea to leave the DOM but tear down (or freeze?) the presentation. The former is what happens for display:none, the latter is what happens for bfcache... Not sure which is better here.
(In reply to comment #64) > I just realized that one other thing to double-check is that the widget and > view (and viewmanager) are being properly reparented. In fact, it might be a > good idea to leave the DOM but tear down (or freeze?) the presentation. The > former is what happens for display:none, the latter is what happens for > bfcache... Not sure which is better here. Which is former and which is latter? Please explain your thoughts in more detail. Do you mean leaving the docshell in the tree and "freezing" it instead? If yes please suggest appropriate code examples to illustrate.
> Which is former and which is latter? Tearing down and freezing respectively. And I was talking about the presentation (presshell and prescontext), not the docshell. I'd explain the thoughts in more detail if there were more detail. I haven't had a chance to think this whole thing through in detail; doing that is part and parcel of fixing this bug, so it's up to the patch writer and perhaps reviewers. Doing a search on the string "freeze(" at http://mxr.mozilla.org/ should work for code examples.
Comment on attachment 308016 [details] [diff] [review] Final Patch More testing needed, reference cycle should be tested and fixed. The patch uses 4 and 2 spaces as indentation. Please be consistent and use the same style as in the file.
Attachment #308016 - Flags: review?(Olli.Pettay) → review-
I tried freezing the presshell. Using SetSticky() and Freeze(). I unstick and Thaw() when re-appending the IFrame back into DOM with AppendChild(). This has created a small problem. Since docshell already has a presshell I get an assert because ShowDocShell() doesn't expect mInnerView to be NULL. See below: -------------------------- nsresult nsSubDocumentFrame::ShowDocShell() { nsCOMPtr<nsIDocShell> docShell; nsresult rv = GetDocShell(getter_AddRefs(docShell)); NS_ENSURE_SUCCESS(rv, rv); nsCOMPtr<nsIPresShell> presShell; docShell->GetPresShell(getter_AddRefs(presShell)); if (presShell) { // The docshell is already showing, nothing left to do... NS_ASSERTION(mInnerView, "What's going on?"); return NS_OK; } ----------------- If I comment out the assert and fill in mInnerView by calling CreateViewAndWidget(contentType); later in the code whenever mInnerView is null I get past the crash. However now my IFrame has nothing in it! The screen is showing a black IFrame. I'm at an impass. Any help would be greatly appreciated. Why does ShowDocShell() expect both presshell and mInnerView to be NULL? I like this solution so far because it keeps docshell's mParentWidget alive. Which allow asyncronous loading to complete normally after the IFrame has been removed from DOM (It behaves exactly like IE does). Thanks. PLEASE HELP.
>The screen is showing a black IFrame. I meant to say blank NOT black.
Dear All, Is this problem solved (Iframe reloads when moved around the DOM tree) and where and how to update the patch. Thank & Best Regards Clive
Just as a reference, I've ran the testcase on other browsers: * Safari 3.1/Win: fails (=reloads the iframes, too) * Opera 9.5/Win: fails * IE 7/Win: passes Sad enough.
Component: DOM: Core → DOM: Core & HTML
QA Contact: ian → general
Is there any workaround for this "Iframe reloads when moved around the DOM tree" bug? It seems has been there for quite a while.
I'm putting this one back in the pool in the hope that somebody who has more expertise and time can be the hero. The mochitest and test cases I added are still solid either way. Smaug, Boris? can you guys step up and finally fix this?
Assignee: MikeM → nobody
Status: ASSIGNED → NEW
If someone else takes other things off my plate, sure... Therein lies the problem.
interesting.. .. ran some tests and noticed that this isn't even just about moving the ELEMENT in the DOM but also, if the CSS selectors for that element are changed also. Using jquery with sortbale DIVS and iframes and using firebug to watch the element, if you move the element slightly, the element never actually moves in the DOM or out of its parent element, however its top,left,height,width and zindex are all changed as well as poition :absolute. hoping this helps in any way towards a solution?
Ran a few more tests it this doesn't appear to be related to the css selectors doh! Must be when jquery makes a call to the DOM's insertBefore / appendAfter. i retract my comments ;) I guess the javascript interpreter and / or firebug are not sufficient enough tools to debug such a low level bug ;) R(In reply to comment #75) > interesting.. .. ran some tests and noticed that this isn't even just about > moving the ELEMENT in the DOM but also, if the CSS selectors for that element > are changed also. > > Using jquery with sortbale DIVS and iframes and using firebug to watch the > element, if you move the element slightly, the element never actually moves in > the DOM or out of its parent element, however its top,left,height,width and > zindex are all changed as well as poition :absolute. > > hoping this helps in any way towards a solution?
There's a glimmer of a work around. Webkit has/had the same bug, but as discussed in their bugzilla ( https://bugs.webkit.org/show_bug.cgi?id=13574#c10 ) they've added new functionality to suppress the reload when adoptNode() is used to move the iframe. This isn't in Mozilla yet (see http://groups.google.com/group/mozilla.dev.tech.dom/browse_thread/thread/960efae1f2ca7bfb?pli=1 ) but when this adoptNode() functionality lands, then you can also use adoptNode() to work around the issue of this bug. (Mozilla bug 284707 and bug 460460 seem related; I couldn't find a bug which explicitly requested implementation of the webkit adoptNode functionality.)
@Scott thx for the links ! The workaround works nice on webkit and IE does not have this issue. Firefox is the only one remaining :( @Boris: Damn... With more and more iFrames on the web lately (Facebook is moving from FBML to iFrames), it would be nice to fix it sooner than "Long-term".
Will we see a fix to adoptNode like webkit based browsers so iframes dont re-execute when moved around the dom!? Would help with preloading third party banners instead of implementing a js solution due to the awful document.write at the end of the page load! As there is a new release of FF every 6 weeks or sooner, lets hope we can fit this in asap!
Would like to see a solution for this, too. Just as another use case where this bug is interfering: WYSIWYG editors (like CKEDITOR) are using iframes for the editor content and moving them around deletes the content, as the iframe is reloaded. That makes it impossible to combine a CKEDITOR with the jQuery sortable plugin, to sort multiple instances of the editor.
Same issue with the Orion editor we are now using as part of browser/devtools.
We could try to specifically hack the "move" operation to not unload the iframe... We'd need to introduce an internal concept of "move" to do that. :( The other option is to actually go through and fix all the places that depend on the docshell tree for security stuff to go through the ownerDocument tree instead.
(In reply to Boris Zbarsky (:bz) from comment #82) > We could try to specifically hack the "move" operation to not unload the > iframe... We'd need to introduce an internal concept of "move" to do that. > :( Doesn't WebKit introduce this concept 'simply' as "an iframe is moved when it is adopted (adoptNode) by the same document" ?
We can't do that without doing the second half of comment 82, since adoptNode is a straight-out removal from the DOM.
(In reply to Boris Zbarsky (:bz) from comment #84) > We can't do that without doing the second half of comment 82, since > adoptNode is a straight-out removal from the DOM. I'm probably overlooking something, so forvige my naive question, but why would it need a "straight-out removal from the DOM" when adoptNode()'s destination document is the same as the iframe's ownerDocument? The ownerDocument being unchanged by such a move, the pervasive docshell tree checks might not be failing and/or necessary (to re-perform).
> but why would it need a "straight-out removal from the DOM" Because the spec says so: http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#concept-node-adopt step 2. Or http://www.w3.org/TR/DOM-Level-3-Core/core.html#Document3-adoptNode third sentence if you prefer your DOM specs old. The point is that after the adoptNode call the iframe is no longer in the document tree. If at any point after that the page in it tries to do something, it needs to be able to reach the right docshells as needed.... even if the node never got reinserted after the adoptNode call.
So how do we fix this issue, as it seems to affect many different user cases? If you could all vote for this bug to be fixed, we might see some movement soon! As it seems to be quite important to have this issue resolved, would Mozilla consider including this fix within the next security update for ALL versions of Firefox??? Reason i ask is some users may not be able to upgrade to the latest version.
(In reply to Tom Taylor from comment #87) > would Mozilla > consider including this fix within the next security update for ALL versions > of Firefox??? No. This is not a critical security or stability bug. And this is quite a huge new feature to implement, so normal Nightly/Aurora/Beta cycles are certainly needed for testing.
(In reply to Olli Pettay [:smaug] from comment #88) > (In reply to Tom Taylor from comment #87) > > would Mozilla > > consider including this fix within the next security update for ALL versions > > of Firefox??? > No. This is not a critical security or stability bug. > > And this is quite a huge new feature to implement, so normal > Nightly/Aurora/Beta cycles are certainly needed for testing. That sounds fair! :) Once the issue has been resolved i will happily help with testing I wouldn't have thought that much was involved, although it makes sense as to why this change has been delayed for so many years!
I haven't been able to find any mention of this, so I'm going to mention it here: In addition to moving an iframe around in the DOM tree, an iframe in Firefox will also reload if it or any of its DOM ancenstors' "position" CSS attribute changes from one value to another. So for instance, I have found that if the iframe is contained in a DIV that starts off having position "relative" and via JavaScript has its position change to "fixed" during the life of the page, that iframe will reload and all navigation context in that iframe will be lost. In my client's distance learning application, for which we have declared to students that Firefox be recommended browser, we commonly have iframes changing between "embedded" (minimized) view and full-screen (maximized) view. This requires switching the "position" attribute of an ancestor DIV of the iframe from "relative" to "fixed". It works perfectly in WebKit-based browsers, but Firefox causes the frame contents to reload. This is a major inconvenience to students who are working through a Flash-based course embedded in the iframe and have focused on a specific chapter, only to find that when they switch to full-screen mode in Firefox their position in the course is lost. I would hope that a fix to this bug of moving an iframe around in the DOM tree would also fix the bug when changing the "position" attribute. Any update on the status of this bug would be most appreciated by my development team, my client, their instructors and their student enrolment. :)
Paul, there is no reload on style changes in anything resembling a recent gecko version (and here Firefox 1 counts as "recent"). There is a reconstruct on CSS boxes, but that should not affect the DOM or session history or other "navigation" data structures. Plug-ins in such an iframe will get restarted in Firefox versions before Firefox 13; that was bug 90268. So chances are, the issue you're running into is fixed in current nightly builds. Worth trying one to make sure.
Thank you Boris for the update! I didn't realize there was a bug specifically about that (I guess I just didn't know how to search for it). I'll check the nightly builds.
It's been a couple of years.. it seems all engines have aligned with Gecko's (and Presto's) behaviour in the meantime! (Also, this used to break Orkut but Orkut is now RIP). (I've tested with new test cases that load either about:blank or a file from the server, modify the document either through appendChild() or document.write() and moves the IFRAME in the DOM either immediately or from a timeout.)
Boris, do you know if I can safely assume browser have aligned here, and we can close this bug as invalid? I'm a bit surprised by this conclusion, sounds a bit too good to be true :)
Flags: needinfo?(bzbarsky)
Doesn't it? I think it's true, though.
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: needinfo?(bzbarsky)
Resolution: --- → INVALID
I'd feel safer if we knew the spec was on our side and had tests in the W3C test suite.. Let's see. https://html.spec.whatwg.org/multipage/embedded-content.html#the-iframe-element says: When an iframe element is inserted into a document that has a browsing context, the user agent must create a nested browsing context, and then process the iframe attributes for the "first time". Interesting spec prose. What does it mean to say "first time" in quotes? I think it means IFRAME attributes should be evaluated every time as if it were the first time - which is good because it means this bug is verified invalid :) - but it's somewhat ambiguous, as in it might mean you only process the attributes for the first time and not later..? The quotes are somewhat weird. Anne, as you're officially recognised "spec brainbox" ((c) Bruce L) - how do you interpret this, and do you think it could/should be clarified? Also, if I want to add tests, where in the web-platform-tests tree would you expect to find them?
Flags: needinfo?(annevk)
If you follow the link for "process the iframe attributes" it does not seem ambiguous to me at all.
Flags: needinfo?(annevk)
Any opinion on where tests should go?
In html/browsers/windows or html/semantics/embedded-content/the-iframe-element I guess. I would ask jgraham or zcorpan.
Flags: needinfo?(james)
If you're testing the "process the iframe attributes for the first time" algorithm, which it sounds like you are, html/semantics/embedded-content/the-iframe-element seems like the right place to me.
Flags: needinfo?(james)
Thanks James, there will be some tests to review very soon :)

I am sorry to wake this up after 7 years, but I felt the urgency to say this:

It is 2022. The Web has evolved. The possibilities that this particular implementation blocks are still literally infinite. We ought to come up with a simple way to preserve the iframe state during a DOM removal.

Please, think of it this way:

  1. Let's all recall iframes are a beautiful old dream for the Web; The Web as a virtual universe where web apps can coexist and collaborate with each other, and we all win if they can be used soundly inside any web page;

  2. Removing elements from the DOM and putting them back is simply a very common operation - it is very often the one way Web Developers simulate movement on the screen. It is the canonical way to implement many sophisticated interactivity patterns, such as features like "sortable lists", "drag and drop", "layers", "animation between parents", you name it;

  3. If whatever element on a web page simply feels like "reloading" every time you move it around, that ought to be considered a very bad UX. A seamless integrated component "going blank and losing all state all of a sudden"; not good.

Conclusion: iframes reload on DOM removal, so it is currently IMPOSSIBLE to implement a wide range of highly interactive web applications using iframes without damaging the user experience!

That said, I feel this discussion needs to be re-heated.

What is the simplest change in the codebase that would enable such use cases in the most safe and seamless way?

Iframes can already kinda move around by using shadow DOM, see https://github.com/whatwg/html/issues/5484#issuecomment-620481794. The right place for this discussion is that repo, btw.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: