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
Dmitry Titov
2009-12-21 19:34:32 PST
Created attachment 45399 [details]
Proposed patch.
style-queue ran check-webkit-style on attachment 45399 [details] without any errors.
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.
(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. 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.
style-queue ran check-webkit-style on attachment 45416 [details] without any errors.
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? 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. (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. (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'. '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. Nice domain name. 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). Created attachment 47240 [details]
Patch v3
(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. 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. |