[webkit-reviews] review denied: [Bug 102012] Disable frame loading instead of throwing exceptions on subtree modifications in ChildFrameDisconnector : [Attachment 173764] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Elliott Sprehn
<esprehn at chromium.org>'s request for review:
Bug 102012: Disable frame loading instead of throwing exceptions on subtree
modifications in ChildFrameDisconnector
https://bugs.webkit.org/show_bug.cgi?id=102012

Attachment 173764: Patch
https://bugs.webkit.org/attachment.cgi?id=173764&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
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.