Bug 102012 – Disable frame loading instead of throwing exceptions on subtree modifications in ChildFrameDisconnector

• Component: Page Loading
• Status: RESOLVED FIXED

[Elliott Sprehn]

Disable frame loading instead of throwing exceptions on subtree modifications in ChildFrameDisconnector

[Elliott Sprehn]

Created attachment 173764 [details]
Patch

[Ojan Vafai]

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.

[Adam Barth]

I'm happy to look, but I'll need to do so when more awake.  :)

[Adam Barth]

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?

[Elliott Sprehn]

(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.

[Elliott Sprehn]

(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.

[Elliott Sprehn]

Created attachment 174012 [details]
Patch

[WebKit Review Bot]

Comment on attachment 174012 [details]
Patch

Clearing flags on attachment: 174012

Committed r134528: <http://trac.webkit.org/changeset/134528>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Elliott Sprehn]

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.

[Ojan Vafai]

(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.