[webkit-reviews] review denied: [Bug 80020] [Crash] Adding <content> into a ShadowRoot causes crash. : [Attachment 131992] Patch

[bugzilla-daemon at webkit.org]

Hajime Morrita <morrita at google.com> has denied Shinya Kawanaka
<shinyak at chromium.org>'s request for review:
Bug 80020: [Crash] Adding <content> into a ShadowRoot causes crash.
https://bugs.webkit.org/show_bug.cgi?id=80020

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

------- Additional Comments from Hajime Morrita <morrita at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=131992&action=review


What makes this patch cryptic is that this changes both lifetime of the
HTMLContentSelector and adding a re-entrant guard in distribution.
Ideally these could be done in separate patch. But I think this is OK for now
to fix crash asap.

> Source/WebCore/ChangeLog:8
> +	   The problem is <content> try to select host children though it is
not prepared.

s/try/tries

> Source/WebCore/dom/ShadowTree.cpp:183
> +    ensureSelector()->willSelect();

It looks we no longer need ensurSelector(). that means we no longer need to
allocate it in the heap.

> Source/WebCore/html/shadow/HTMLContentSelector.cpp:167
> +    ASSERT(m_phase == NotPopulatedHostChildren);

This looks too obvious.

> Source/WebCore/html/shadow/HTMLContentSelector.h:147
> +	   NotPopulatedHostChildren,

Please consider to avoid negation.

> Source/WebCore/html/shadow/InsertionPoint.cpp:115
> +	   selector->select(this, &m_selections);

Please consider early return. I think we can just return unless the selector is
available.