Bug 52018 – Adopting an iframe to a child frame results in stack overflow

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

Created attachment 78163 [details]
demo

Reproduction steps:
1. Create an iframe A
2. Create an iframe B under A (i.e. B is a child of A).
3. B.contentDocument.body.appendChild(B.contentDocument.adoptNode(A));

[Ryosuke Niwa]

Stack trace:
#0	0x1018aef7b in WebCore::FrameLoader::subframeIsLoading at FrameLoader.cpp:2252
#1	0x10174e652 in WebCore::DocumentLoader::isLoadingInAPISense at DocumentLoader.cpp:436
#2	0x1018aefa5 in WebCore::FrameLoader::subframeIsLoading at FrameLoader.cpp:2254
#3	0x10174e652 in WebCore::DocumentLoader::isLoadingInAPISense at DocumentLoader.cpp:436

[Ryosuke Niwa]

Created attachment 78202 [details]
Patch

[Ojan Vafai]

Comment on attachment 78202 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78202&action=review

> WebCore/dom/Document.cpp:896
> +            if (frame()->tree()->isDescendantOf(iframe->contentFrame())) {

Does this cross-frame boundaries correctly? For example, lets say I have 3 frames. F1, F2, F3. F2 is a child of F1 and F3 is a child of F2. Will this catch the case where I try to F3.contentDocument.adoptNode(F1)?

[Ryosuke Niwa]

Comment on attachment 78202 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78202&action=review

>> WebCore/dom/Document.cpp:896

> 
> Does this cross-frame boundaries correctly? For example, lets say I have 3 frames. F1, F2, F3. F2 is a child of F1 and F3 is a child of F2. Will this catch the case where I try to F3.contentDocument.adoptNode(F1)?

Yes. isDescendantOf works just like Node's isDescendantOf except that FrameTree's isDescendantOf works on the frame hierarchy.  That said, maybe I should be adding some more tests.

[Ryosuke Niwa]

Created attachment 78269 [details]
Added one more test case per Ojan's comment

[Ojan Vafai]

Comment on attachment 78269 [details]
Added one more test case per Ojan's comment

View in context: https://bugs.webkit.org/attachment.cgi?id=78269&action=review

What do other browsers do in this case?

> LayoutTests/fast/html/adopt-parent-frame.html:21
> +    iframe.contentDocument.body.appendChild(iframe.contentDocument.createElement('br'));
> +    iframe.style.width = '70%';
> +    iframe.style.height = '40%';

These three lines don't seem useful. Delete them?

[Ryosuke Niwa]

Comment on attachment 78269 [details]
Added one more test case per Ojan's comment

View in context: https://bugs.webkit.org/attachment.cgi?id=78269&action=review

>> LayoutTests/fast/html/adopt-parent-frame.html:21

> 
> These three lines don't seem useful. Delete them?

Adding width and height will make it easier to see the structure of iframes.  Without it, each iframe will be of the same size and hard to see when you're debugging manually.  But I'll remove if you're strongly against having them.

[Ojan Vafai]

(In reply to comment #7)
> (From update of attachment 78269 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78269&action=review
> 
> >> LayoutTests/fast/html/adopt-parent-frame.html:21
> Adding width and height will make it easier to see the structure of iframes.  Without it, each iframe will be of the same size and hard to see when you're debugging manually.  But I'll remove if you're strongly against having them.

Meh. I don't feel strongly. I don't really see how it helps given that your inserting the ID of the frame into it's contents, but whatever.

[Ryosuke Niwa]

(In reply to comment #6)
> (From update of attachment 78269 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78269&action=review
> 
> What do other browsers do in this case?

Our behavior matches that of Firefox.

[Ryosuke Niwa]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 78269 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=78269&action=review
> > 
> > >> LayoutTests/fast/html/adopt-parent-frame.html:21
> > Adding width and height will make it easier to see the structure of iframes.  Without it, each iframe will be of the same size and hard to see when you're debugging manually.  But I'll remove if you're strongly against having them.
> 
> Meh. I don't feel strongly. I don't really see how it helps given that your inserting the ID of the frame into it's contents, but whatever.

Yeah, but having width/height makes it so that the tree structure shows up visually.  i.e. you can see which frames resides in which.  I think that's more intuitive than having to open up an inspector and probing through the DOM tree.

[Ryosuke Niwa]

I'll land a patch after adding a line that says new behavior matches that of Firefox in the change log entry.

[Ryosuke Niwa]

Committed r75293: <http://trac.webkit.org/changeset/75293>

[Ryosuke Niwa]

This patch caused fast/frames/iframe-reparenting.html to crash because frame() could be null.  Fixed in http://trac.webkit.org/changeset/75307.

[Adam Barth]

I'm removing this special case in Bug 99247 now that we no longer support magic iframe.

[Ryosuke Niwa]

(In reply to comment #14)
> I'm removing this special case in Bug 99247 now that we no longer support magic iframe.

Makes sense!