Bug 99247 – Document::adoptNode shouldn't special-case <iframe>

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

Document::adoptNode shouldn't special-case <iframe>

[Adam Barth]

Created attachment 168550 [details]
Patch

[Build Bot]

Comment on attachment 168550 [details]
Patch

Attachment 168550 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14286187

New failing tests:
fast/frames/adopt-iframe-into-itself.html

[WebKit Review Bot]

Comment on attachment 168550 [details]
Patch

Attachment 168550 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14297210

New failing tests:
fast/frames/adopt-iframe-into-itself.html

[Ojan Vafai]

Comment on attachment 168550 [details]
Patch

It's not clear from the test or the code, what do we actually do in this case now? The adoptNode silently fails?

[Ryosuke Niwa]

Comment on attachment 168550 [details]
Patch

Please fix the test before landing it.

[Adam Barth]

> It's not clear from the test or the code, what do we actually do in this case now? The adoptNode silently fails?

It succeeds in detaching the iframe from the DOM, but that process tears down the document inside the frame, so nothing very interesting happens.

[Ojan Vafai]

So, we set the m_document of the ancestor iframe to the document in one of its descendant frames. We don't actually teardown the inner document until we destroy the iframe. At a theoretical level, the old code looks correct to me. It seems non-sensical to set a frame's document to one of it's descendant frames' document. For example, now you can appendChild an iframe to one of it's descendant frames.

At a practical level, I can't imagine there are sites that depend on this behavior. I would guess that other browsers throw an exception in this case though. FWIW, I tested Firefox and they throw a TypeError in this case.

I agree that this code shouldn't special case iframe, but maybe it should be for frames in general (i.e. anything we put in the frame tree). That seems more correct to me.

I don't feel strongly about this, so go ahead if you want to with rniwa's R+. This just seems like a step in the (slightly) wrong direction to me.

FWIW, I think allowing the adoptNode to succeed may expose a possible, albeit very unlikely, memory leak if you were to append the iframe to the newly adopted document.  The new document is now being guardRef'ed by the iframe, and the iframe is being kept alive by virtue of having a parentNode.

[Adam Barth]

Created attachment 169012 [details]
Patch

[WebKit Review Bot]

Comment on attachment 169012 [details]
Patch

Clearing flags on attachment: 169012

Committed r131500: <http://trac.webkit.org/changeset/131500>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.