[webkit-reviews] review requested: [Bug 36675] Re-entrant layout via plug-ins may cause crashes with bad RenderWidgets : [Attachment 51780] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has asked  for review:
Bug 36675: Re-entrant layout via plug-ins may cause crashes with bad
RenderWidgets
https://bugs.webkit.org/show_bug.cgi?id=36675

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +    typedef Vector<RenderEmbeddedObject*> RenderEmbeddedObjectVector;

I don't think this typedef is worthwhile. We only use the type in one place.

> +    RenderEmbeddedObjectVector renderEmbeddedObjects;

I think just "renderers" or "objects" would be a fine name for this. It's just
a local variable. The new name seems too wordy to me.

> +    renderEmbeddedObjects.reserveCapacity(m_widgetUpdateSet->size());
> +
> +    RenderEmbeddedObjectSet::const_iterator end = m_widgetUpdateSet->end();
> +    for (RenderEmbeddedObjectSet::const_iterator it =
m_widgetUpdateSet->begin(); it != end; ++it) {
> +	   renderEmbeddedObjects.uncheckedAppend(*it);
> +	   (*it)->ref();
> +    }
> +
> +    size_t size = renderEmbeddedObjects.size();

I suggest setting this up earlier, initializing it to
m_widgetUpdateSet->size(), and using it for the reserveCapacity call above.

Another possibility would be to create the vector with a fixed size instead of
a fixed capacity, and assign the values instead of using uncheckedAppend.

> +	   // Check that the object hasn't been destroyed by checking the node,
which is nulled
> +	   // out in RenderWidget::destroy. Our manual ref() keeps the object
from being deleted.
> +	   if (object->node())
> +	       object->updateWidget(false);

It's a little strange that we check for a node of 0 directly. Couldn't
updateWidget check instead?

> +	   if (object->node())
>	       object->updateWidgetPosition();

Same question.

> +    , m_bestTruncatedAt(0)
> +    , m_truncatorWidth(0)
> +    , m_minimumColumnHeight(0)
> +    , m_forcedPageBreak(false)

I guess this is OK. I'd prefer if these were set to bad "don't use this" values
instead of good-seeming ones, but integers and booleans don't offer such
values.

> +    typedef Vector<RenderWidget*> WidgetVector;

I don't think this typedef is worthwhile. We only use the type in one place.

> +    WidgetVector renderWidgets;
> +    renderWidgets.reserveCapacity(m_widgets.size());
> +
> +    RenderWidgetSet::const_iterator end = m_widgets.end();
> +    for (RenderWidgetSet::const_iterator it = m_widgets.begin(); it != end;
++it) {
> +	   renderWidgets.uncheckedAppend(*it);
> +	   (*it)->ref();
> +    }
> +    
> +    size_t size = renderWidgets.size();

I suggest setting this up earlier, initializing it to m_widgets.size(), and
using it for the reserveCapacity call above.

Another possibility would be to create the vector with a fixed size instead of
a fixed capacity, and assign the values instead of using uncheckedAppend.

> +    RenderArena* ref() { ++m_refCount; return renderArena(); }
> +    void deref(RenderArena*);

It's disappointing that this function needs to be public. This is an example of
a hard-to-use-correctly function, and it's good how having these functions be
private limits the code you need to search to see they're being used correctly.