Bug 52018 - Adopting an iframe to a child frame results in stack overflow
Summary: Adopting an iframe to a child frame results in stack overflow
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-06 14:30 PST by Ryosuke Niwa
Modified: 2012-10-13 10:07 PDT (History)
5 users (show)

See Also:


Attachments
demo (921 bytes, text/html)
2011-01-06 14:30 PST, Ryosuke Niwa
no flags Details
Patch (4.86 KB, patch)
2011-01-06 18:39 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Added one more test case per Ojan's comment (5.20 KB, patch)
2011-01-07 14:25 PST, Ryosuke Niwa
ojan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-01-06 14:30:26 PST
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));
Comment 1 Ryosuke Niwa 2011-01-06 16:49:40 PST
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
Comment 2 Ryosuke Niwa 2011-01-06 18:39:16 PST
Created attachment 78202 [details]
Patch
Comment 3 Ojan Vafai 2011-01-06 18:45:23 PST
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)?
Comment 4 Ryosuke Niwa 2011-01-06 18:49:04 PST
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.
Comment 5 Ryosuke Niwa 2011-01-07 14:25:27 PST
Created attachment 78269 [details]
Added one more test case per Ojan's comment
Comment 6 Ojan Vafai 2011-01-07 15:25:24 PST
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?
Comment 7 Ryosuke Niwa 2011-01-07 15:33:42 PST
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.
Comment 8 Ojan Vafai 2011-01-07 15:35:24 PST
(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.
Comment 9 Ryosuke Niwa 2011-01-07 15:39:06 PST
(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.
Comment 10 Ryosuke Niwa 2011-01-07 15:40:36 PST
(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.
Comment 11 Ryosuke Niwa 2011-01-07 15:41:58 PST
I'll land a patch after adding a line that says new behavior matches that of Firefox in the change log entry.
Comment 12 Ryosuke Niwa 2011-01-07 16:50:07 PST
Committed r75293: <http://trac.webkit.org/changeset/75293>
Comment 13 Ryosuke Niwa 2011-01-07 18:43:19 PST
This patch caused fast/frames/iframe-reparenting.html to crash because frame() could be null.  Fixed in http://trac.webkit.org/changeset/75307.
Comment 14 Adam Barth 2012-10-13 01:58:56 PDT
I'm removing this special case in Bug 99247 now that we no longer support magic iframe.
Comment 15 Ryosuke Niwa 2012-10-13 10:07:29 PDT
(In reply to comment #14)
> I'm removing this special case in Bug 99247 now that we no longer support magic iframe.

Makes sense!