[webkit-reviews] review denied: [Bug 88148] REGRESSION(r118098): <content> element does not render distributed children when cloned from another document : [Attachment 146473] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Hajime Morrita <morrita at google.com>'s
request for review:
Bug 88148: REGRESSION(r118098): <content> element does not render distributed
children when cloned from another document
https://bugs.webkit.org/show_bug.cgi?id=88148

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=146473&action=review


This is a big improvement over the previous version, but I think I've spotted
one bug that you'll probably want to fix before landing.

> Source/WebCore/dom/ContextFeatureSwitch.h:40
> +class ContextFeatureSwitch : public Supplement<Page> {

So, I think there's a subtle bug here.	ContextFeatureSwitch has the same
lifetime as Page, but SecurityContext holds a raw pointer to a
ContextFeatureSwitch.  Document is a subclass of SecurityContext and can
outlive the page.  When it does that, the Document will have a garbage pointer
to a ContextFeatureSwitch.  It will then use-after-free a bunch of memory and
call a virtual method of ContextFeatureSwitchClient, which is a security
vulnerability.

I wonder if a better approach is to make ContextFeatureSwitch RefCounted so
that SecurityContext can hold a reference to it.  You'll then probably want to
make it a FrameDestructionObserver so that it can observe when the Frame goes
away and stop calling through to the client at that point.  (Once the Frame
goes away, there's no reason to believe anything related to the Frame, like the
Page or the ContextFeatureSwitchClient still exists.)