Disable frame loading instead of throwing exceptions on subtree modifications in ChildFrameDisconnector
Created attachment 173764 [details] Patch
Comment on attachment 173764 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173764&action=review If possible, I'd like Adam to take a look at this since he's more familiar with frame loading. Looks good to me though. > Source/WebCore/ChangeLog:16 > + This works because either the subtree will be removed and the frame never > + loading doesn't matter, or some section of the subtree that contains the > + frame will be moved to another part of the document which will cause the > + frame to load when it's inserted there. Kind of "works". Better than the old behavior for sure, but we don't actually load the frame and in theory we could. > Source/WebCore/ChangeLog:25 > + An better fix could be to repeatedly walk the subtree until all frames > + were disconnected or some iteration limit was hit and then force all leftover > + subframes to disconnect without firing unload handlers but this is such an > + edge case I don't think the complexity is necessary. If we keep a count of the unload handlers in the subtree, then we can actually do this properly. Once we've fired all the unload handlers, then we can load the subframes. Might even be worth adding a FIXME for this. I'm OK with this being broken for now, but it's worth documenting that we have a bug in the code. > Source/WebCore/ChangeLog:27 > + No new tests, this is covered by existing tests. Can you add a test for the case where we used too throw an exception for moving non-frame nodes during unload handlers? The iframe node case seems to be handled by the existing test.
I'm happy to look, but I'll need to do so when more awake. :)
Comment on attachment 173764 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173764&action=review > Source/WebCore/dom/Node.cpp:-1202 > - if (newParent->inDocument() && ChildFrameDisconnector::nodeHasDisconnector(newParent)) { > - ec = NO_MODIFICATION_ALLOWED_ERR; > - return; > - } This seems way more general than just frames. Are there other cases we need to worry about?
(In reply to comment #2) > (From update of attachment 173764 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173764&action=review >... > > Source/WebCore/ChangeLog:25 > > + An better fix could be to repeatedly walk the subtree until all frames > > + were disconnected or some iteration limit was hit and then force all leftover > > + subframes to disconnect without firing unload handlers but this is such an > > + edge case I don't think the complexity is necessary. > > If we keep a count of the unload handlers in the subtree, then we can actually do this properly. Once we've fired all the unload handlers, then we can load the subframes. Might even be worth adding a FIXME for this. I'm OK with this being broken for now, but it's worth documenting that we have a bug in the code. That still requires a loop and some fallback behavior to prevent infinite loops. For example if these new frames have unload handlers we have to keep running the disconnect until there's no more frames. You end up with code like: for (i = 0; i < maxIterations && hasConnectedFrames(); i++) disconnectAllSubframes(); if (hasConnectedFrames()) disconnectWithFrameLoadingDisabled(); > > > Source/WebCore/ChangeLog:27 > > + No new tests, this is covered by existing tests. > > Can you add a test for the case where we used too throw an exception for moving non-frame nodes during unload handlers? The iframe node case seems to be handled by the existing test. Sure.
(In reply to comment #4) > (From update of attachment 173764 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173764&action=review > > > Source/WebCore/dom/Node.cpp:-1202 > > - if (newParent->inDocument() && ChildFrameDisconnector::nodeHasDisconnector(newParent)) { > > - ec = NO_MODIFICATION_ALLOWED_ERR; > > - return; > > - } > > This seems way more general than just frames. Are there other cases we need to worry about? Nope. The point of this patch is to relax the restriction but still keep the security fix that this code was added for.
Created attachment 174012 [details] Patch
Comment on attachment 174012 [details] Patch Clearing flags on attachment: 174012 Committed r134528: <http://trac.webkit.org/changeset/134528>
All reviewed patches have been landed. Closing bug.
After some testing it seems Gecko calls unload *after* the subtree removal so they won't load frames either: <div> <iframe></iframe> </div> <script> onload = function() { div = document.querySelector('div'); frame = document.querySelector('iframe'); frame.contentWindow.onunload = function() { console.log(top.frame.parentNode.parentNode); // In Gecko the div is already removed so parentNode == null // In WebKit we do this before the real remove so parentNode == body frame = div.appendChild(document.createElement('iframe')); console.log(frame.contentDocument); // In Gecko and WebKit (after r134528) frame.contentDocument == null // In older WebKit the appendChild throws a NO_MODIFICATION_ALLOWED_ERR }; div.parentNode.removeChild(div); }; </script> So this patch made us match Gecko by not loading frames.
(In reply to comment #10) > After some testing it seems Gecko calls unload *after* the subtree removal so they won't load frames either: Unfortunately, IE9/10 do the unload before the subtree removal. I couldn't get Opera to fire unload events at all from iframes. Maybe we should try having the unload fire after the frame has been unloaded. We can do it behind a flag to start with so Chromium can ship it as a way of vetting it's safety before turning it on for other ports.