Bug 32848

Summary: Avoid reloading iframe on re-parenting between documents.
Product: WebKit Reporter: Dmitry Titov <dimich>
Component: WebCore Misc.Assignee: Dmitry Titov <dimich>
Status: RESOLVED FIXED    
Severity: Normal CC: aldon.mackie, dglazkov, fishd, ian, levin, mitz, mjs, ojan, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Proposed patch.
dimich: commit-queue-
Proposed patch, v2.
levin: review-, dimich: commit-queue-
Patch v3 levin: review+, dimich: commit-queue-

Dmitry Titov
Reported 2009-12-21 19:34:32 PST
As a follow-up to SharedScript discussion, we need to implement a behavior change for iframe to address the use cases requiring passing 'hot' content between windows and frames. Today, when iframe is removed from the tree of one document and appended to the tree of another, the content gets unloaded and loaded again, onload handlers fire, timers and XHRs get killed etc. This behavior is not specified anywhere in the HTML5 spec though and therefore making iframe to skip reloading was proposed as a replacement to more intrusive SharedScript. However, just making iframe keep active state after removing from the tree is bound to cause backward compatibility issues (quick test indicates about two dozen layout tests fail/crash). One way to narrow down the effect of the change is to only keep the content of iframe alive when it is explicitly moved from one document to another using docment.adoptNode() API, for example, to move iframe from document1 to document2: document2.adoptNode(iframe); // This does removeChild() internally but does not unload content. document2.body.appendChild(iframe); The implementation plan is to set a flag in adoptNode() on the iframe if the passed node is an iframe. This flag will prevent the iframe from unloading during internal removeChild() and then cause to skip loading during appendChild(). Once attached to another document's tree, the protection bit is reset. This way, the regular operations on the tree would have old behavior while only the sequence including adoptNode() directly on iframe element would behave different.
Attachments
Proposed patch. (10.83 KB, patch)
2009-12-22 13:02 PST, Dmitry Titov 		dimich: commit-queue-
DetailsFormatted DiffDiff
Proposed patch, v2. (13.71 KB, patch)
2009-12-22 20:52 PST, Dmitry Titov 		levin: review-
dimich: commit-queue-
DetailsFormatted DiffDiff
Patch v3 (14.25 KB, patch)
2010-01-22 15:37 PST, Dmitry Titov 		levin: review+
dimich: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Dmitry Titov
Comment 1 2009-12-22 13:02:44 PST
Created attachment 45399 [details] Proposed patch.
WebKit Review Bot
Comment 2 2009-12-22 13:06:14 PST
style-queue ran check-webkit-style on attachment 45399 [details] without any errors.
Darin Adler
Comment 3 2009-12-22 13:18:23 PST
Comment on attachment 45399 [details] Proposed patch. What happens in the case where the frame is removed added back into another document, but it's a document that is not rendered and so attach is never called? Specifically, we should have a test case where we adopt the node into a document loaded by XMLHttpRequest. I suggest having keepAliveOnRemovalFromTree() be protected rather than public.
Dmitry Titov
Comment 4 2009-12-22 14:16:06 PST
(In reply to comment #3) > (From update of attachment 45399 [details]) > What happens in the case where the frame is removed added back into another > document, but it's a document that is not rendered and so attach is never > called? Specifically, we should have a test case where we adopt the node into a > document loaded by XMLHttpRequest. Good point. My first thought would be we don't want the iframe to be alive when it's adopted into a document that is not rendered, because it's unclear how to support things requiring UI, like http authentication dialog etc. Will add a check and a test. > > I suggest having keepAliveOnRemovalFromTree() be protected rather than public. Will do.
Dmitry Titov
Comment 5 2009-12-22 20:52:03 PST
Created attachment 45416 [details] Proposed patch, v2. 1. Added check for document being 'attached()', and added a test where document loaded by XMLHttpRequest is used to adopt the iframe, verifying that iframe still unloads in this scenario. 2. Moved keepAliveOnRemovalFromTree() to 'protected' from 'public'. 3. Added a one-shot timer that makes sure that if script did adoptNode() but did not added the iframe into the tree, the iframe gets unloaded - to avoid active content running in non-attached iframe. This can cause more issues and memory leaks. I think enabling live iframe reparenting opens up some space for issues - for example, I think the frame tree is not maintained right and other issues perhaps are still to be discovered once the active reparenting is actually used. It might be easier and faster to move step by step so if this patch is reasonable then I'd add more tests and fixes in separate patches, to keep things smaller. Obviously, one thing to look for would be compatibility issues.
WebKit Review Bot
Comment 6 2009-12-22 20:54:42 PST
style-queue ran check-webkit-style on attachment 45416 [details] without any errors.
mitz
Comment 7 2009-12-23 22:02:17 PST
It would be funny if this bug was addressed and bug 13574 wasn’t. Is there a reason to prevent unloading only in the inter-document case and not in the intra-document case?
Ojan Vafai
Comment 8 2009-12-24 09:41:29 PST
What if we exposed something to the DOM that allowed iframes to render when not appended to the DOM, e.g. a rendersWhenOrphaned attribute that defaults to false? This would allow both the inter-document and intra-document cases to work, but it would also allow apps to preload iframes before appending them to the DOM.
Dmitry Titov
Comment 9 2009-12-24 19:27:12 PST
(In reply to comment #7) > It would be funny if this bug was addressed and bug 13574 wasn’t. Is there a > reason to prevent unloading only in the inter-document case and not in the > intra-document case? No reason other then this is a change in behavior that shipped long ago and is the same in FF. Chances are changing this behavior will have compat issues. It makes sense to move step by step and first limit the behavior change to adoptNode()/appendChild() case, since it lets play with the feature, write tests, find and fix bugs without exposing it too much. Note intra-document cases also are covered by adoptNode(). I think 'atomic' reparenting by a single appendChild() etc can be easily enabled later, I'd like to have 'live iframe' more mature when it happens.
Dmitry Titov
Comment 10 2010-01-04 20:28:49 PST
(In reply to comment #8) > What if we exposed something to the DOM that allowed iframes to render when not > appended to the DOM, e.g. a rendersWhenOrphaned attribute that defaults to > false? This would allow both the inter-document and intra-document cases to > work, but it would also allow apps to preload iframes before appending them to > the DOM. It seems adding such a property is similar to having SharedScript, but with added burden of visual aspect of the page (layout, JS queries of computed values etc). If there is a 'mode' of iframe when it's always alive, even while not attached to a tree of a rendering document, then there is much bigger surface to define/spec/implement etc. Examples are layout size, UI features like alert() and auth, etc. The SharedScript was an attempt to have a much simpler, JS-only context to run 'orphaned', or independently - to avoid this kind of issues. The current idea is to have the iframe to survive 'atomic reparenting' - when the iframe is re-attached into another (or same) document by means of a single appendChild() or a combination of adoptNode()/appendChild() in a single 'run' of JS, which in current single-threaded DOM implementations is practically 'atomic'.
Dmitry Titov
Comment 11 2010-01-07 18:14:26 PST
'demo page' for the feature: http://figushki.com/test/magic-iframe/main.html The page loads iframe and starts a timer in it. Clicking 'reparent magic iframe' button does adoptNode/appendChild. Currently in FF and WebKit, it reloads the iframe. After the patch, it keeps the timer ticking (and XHR loading) after re-parenting. Interesting, IE does not have adoptNode(), and does not allow moving iframe between documents (appendChild throws exception), which I think reduces compat risk surface even more.
Dimitri Glazkov (Google)
Comment 12 2010-01-08 08:00:46 PST
Nice domain name.
David Levin
Comment 13 2010-01-22 12:32:12 PST
Comment on attachment 45416 [details] Proposed patch, v2. So far I've focused on the WebCore code and not the test code but I'll look at the test more soon. Just wanted to send this feedback along first. > diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog > +2009-12-22 Dmitry Titov <dimich@chromium.org> > + > + Reviewed by NOBODY (OOPS!). > + > + Avoid reloading iframe on re-parenting between documents. > + https://bugs.webkit.org/show_bug.cgi?id=32848 Nice to have a blank line here. (I always see that information like a title.) > diff --git a/WebCore/dom/Document.cpp b/WebCore/dom/Document.cpp > + if (source->hasTagName(iframeTag) && attached()) > + static_cast<HTMLIFrameElement*>(source.get())->setKeepAliveOnRemovalFromTree(true); Shouldn't this be: if (source->hasTagName(iframeTag)) static_cast<HTMLIFrameElement*>(source.get())->setKeepAliveOnRemovalFromTree(attached()); This handles the case of a node being adopted by an attached frame and then adopted by an unattached frame right after (maybe the test should be adjusted to cover this case as well?). > diff --git a/WebCore/html/HTMLFrameElementBase.cpp b/WebCore/html/HTMLFrameElementBase.cpp > + if (!keepAliveOnRemovalFromTree()) > + queuePostAttachCallback(&HTMLFrameElementBase::setNameAndOpenURLCallback, this); > + setKeepAliveOnRemovalFromTree(false); Why does this only get set to false inside of this if? > diff --git a/WebCore/html/HTMLFrameOwnerElement.cpp b/WebCore/html/HTMLFrameOwnerElement.cpp > +void HTMLFrameOwnerElement::setKeepAliveOnRemovalFromTree(bool value) > +{ > + m_keepAliveOnRemovalFromTree = value; > + > + // There is a possibility that JS will do document.adoptNode() on this element but will not insert it into the tree. > + // Check asynchronously and unload the frame if needed. > + if (value) > + m_unloadTimer.startOneShot(0); m_unloadTimer -- the name bothers me because it really is about checking to see if the Frame got attached. > + else > + m_unloadTimer.stop(); > +} > + > +void HTMLFrameOwnerElement::detachTimerFired(Timer<HTMLFrameOwnerElement>*) > +{ > + if (attached()) > + return; > + m_keepAliveOnRemovalFromTree = false; It seems like this should be before the "if" to reset the value of m_keepAliveOnRemovalFromTree. > + willRemove(); > +} > + > HTMLFrameOwnerElement::~HTMLFrameOwnerElement() > { > if (m_contentFrame) > diff --git a/WebCore/html/HTMLFrameOwnerElement.h b/WebCore/html/HTMLFrameOwnerElement.h > SandboxFlags sandboxFlags() const { return m_sandboxFlags; } > - > + void setKeepAliveOnRemovalFromTree(bool); > + > protected: > HTMLFrameOwnerElement(const QualifiedName& tagName, Document*); > > void setSandboxFlags(SandboxFlags); > - > + > + bool keepAliveOnRemovalFromTree() const { return m_keepAliveOnRemovalFromTree; } I've started considering how method names read when prefixed by the class name. HTMLFrameOwnerElement keepAliveOnRemovalFromTree is ok, but HTMLFrameOwnerElement remainsAliveWhenRemovedFromTree reads much better imo. > Frame* m_contentFrame; > SandboxFlags m_sandboxFlags; > + Timer<HTMLFrameOwnerElement> m_unloadTimer; > + bool m_keepAliveOnRemovalFromTree; Consider moving the m_keepAliveOnRemovalFromTree to be right after the m_sandboxFlags (which is an int to help avoid possible padding holes on 64bit). Actually I'd recommend moving this stuff into WebCore/html/HTMLFrameElementBase.cpp if possible (and moving the bool by other bool's in there).
Dmitry Titov
Comment 14 2010-01-22 15:37:05 PST
Created attachment 47240 [details] Patch v3
Dmitry Titov
Comment 15 2010-01-22 15:42:17 PST
(In reply to comment #13) > (From update of attachment 45416 [details]) > So far I've focused on the WebCore code and not the test code but I'll look at > the test more soon. Just wanted to send this feedback along first. Uploaded the updated patch, the test is the same, WebCore code updated. > > + Avoid reloading iframe on re-parenting between documents. > > + https://bugs.webkit.org/show_bug.cgi?id=32848 > > Nice to have a blank line here. (I always see that information like a title.) Added blank line. > > + if (source->hasTagName(iframeTag) && attached()) > > + static_cast<HTMLIFrameElement*>(source.get())->setKeepAliveOnRemovalFromTree(true); > > Shouldn't this be: > > if (source->hasTagName(iframeTag)) > > static_cast<HTMLIFrameElement*>(source.get())->setKeepAliveOnRemovalFromTree(attached()); > > This handles the case of a node being adopted by an attached frame and then > adopted by an unattached frame right after (maybe the test should be adjusted > to cover this case as well?). Good catch! Done. > > diff --git a/WebCore/html/HTMLFrameElementBase.cpp b/WebCore/html/HTMLFrameElementBase.cpp > > + if (!keepAliveOnRemovalFromTree()) > > + queuePostAttachCallback(&HTMLFrameElementBase::setNameAndOpenURLCallback, this); > > + setKeepAliveOnRemovalFromTree(false); > > Why does this only get set to false inside of this if? Agree, moved outside. > > +void HTMLFrameOwnerElement::setKeepAliveOnRemovalFromTree(bool value) > > +{ > > + m_keepAliveOnRemovalFromTree = value; > > + > > + // There is a possibility that JS will do document.adoptNode() on this element but will not insert it into the tree. > > + // Check asynchronously and unload the frame if needed. > > + if (value) > > + m_unloadTimer.startOneShot(0); > > m_unloadTimer -- the name bothers me because it really is about checking to see > if the Frame got attached. renamed to checkAttachTimer. Perhaps this is better. HTMLFrameOwnerElement::detachTimerFired(Timer<HTMLFrameOwnerElement>*) > > +{ > > + if (attached()) > > + return; > > + m_keepAliveOnRemovalFromTree = false; > > It seems like this should be before the "if" to reset the value of > m_keepAliveOnRemovalFromTree. Re-wrote this. No need to check for attached(), actually - since timer only fires if attach() was not called. Added ASSERT(attached()) though. > > + bool keepAliveOnRemovalFromTree() const { return m_keepAliveOnRemovalFromTree; } > > I've started considering how method names read when prefixed by the class name. > > HTMLFrameOwnerElement keepAliveOnRemovalFromTree is ok, but > > HTMLFrameOwnerElement remainsAliveWhenRemovedFromTree reads much better imo. Renamed. > Actually I'd recommend moving this stuff into > WebCore/html/HTMLFrameElementBase.cpp if possible (and moving the bool by other > bool's in there). Moved all the code to HTMLFrameElementBase.
David Levin
Comment 16 2010-01-22 16:12:07 PST
Comment on attachment 47240 [details] Patch v3 OK the js test looks fine. One nit below. > diff --git a/WebCore/html/HTMLFrameElementBase.cpp b/WebCore/html/HTMLFrameElementBase.cpp > @@ -50,8 +50,11 @@ HTMLFrameElementBase::HTMLFrameElementBase(const QualifiedName& tagName, Documen > , m_scrolling(ScrollbarAuto) > , m_marginWidth(-1) > , m_marginHeight(-1) > + , m_checkAttachedTimer(this, &HTMLFrameElementBase::checkAttachedTimerFired) > , m_viewSource(false) > , m_shouldOpenURLAfterAttach(false) > + , m_remainsAliveOnRemovalFromTree(false) > + Nit: Extra blank line here. > +void HTMLFrameElementBase::checkAttachedTimerFired(Timer<HTMLFrameElementBase>*) > +{ > + ASSERT(!attached()); Ah, so this enforces that setRemainsAliveOnRemovalFromTree is only called during a remove operation (and that attach cancels the timer) which seems fine.
Dmitry Titov
Comment 17 2010-01-26 16:29:51 PST
Landed: http://trac.webkit.org/changeset/53871
Note You need to log in before you can comment on or make changes to this bug.