| Summary: | Adopting an iframe to a child frame results in stack overflow | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||
| Component: | DOM | Assignee: | Nobody <webkit-unassigned> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | abarth, ap, darin, dbates, ojan | ||||||||
| Priority: | P1 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Attachments: |
|
||||||||||
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 Created attachment 78202 [details]
Patch
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 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. Created attachment 78269 [details]
Added one more test case per Ojan's comment
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 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. (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. (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. (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. I'll land a patch after adding a line that says new behavior matches that of Firefox in the change log entry. Committed r75293: <http://trac.webkit.org/changeset/75293> This patch caused fast/frames/iframe-reparenting.html to crash because frame() could be null. Fixed in http://trac.webkit.org/changeset/75307. I'm removing this special case in Bug 99247 now that we no longer support magic iframe. (In reply to comment #14) > I'm removing this special case in Bug 99247 now that we no longer support magic iframe. Makes sense! |
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));