Bug 36675 – Re-entrant layout via plug-ins may cause crashes with bad RenderWidgets

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Simon Fraser (smfr)]

There are a couple of code paths via which RenderWidgets can be destroyed while being iterated over; one is via re-entering RenderView::updateWidgetPositions(), which was observed on hulu.com, and another which hits FrameView::updateWidgets().

[Simon Fraser (smfr)]

<rdar://problem/7787617>

[Simon Fraser (smfr)]

Created attachment 51780 [details]
Patch

[WebKit Review Bot]

Attachment 51780 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/rendering/RenderWidget.h:50:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

+    typedef Vector<RenderEmbeddedObject*> RenderEmbeddedObjectVector;

If you used "Vector<RefPtr<RenderEmbeddedObject>, 16>
RenderEmbeddedObjectVector" instead, you could remove some boilerplate
ref/deref code, and possibly avoid malloc overhead too.

[Simon Fraser (smfr)]

RenderEmbeddedObject is manually ref-counted, not RefCounted, so I can't use RefPtr.

Comment on attachment 51780 [details]
Patch


> +        vector before iterating of them, and dref() them at the end. Rather than checking the m_widgetUpdateSet

Fatal typo: dref().

It’s sad that you can’t use something like RenderWidgetProtector here, but this seems to be more efficient.

[Darin Adler]

Comment on attachment 51780 [details]
Patch

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

[Darin Adler]

Comment on attachment 51780 [details]
Patch

Oops, meant to leave this as r=mitz.

I agree with Darin’s comments :)

[Simon Fraser (smfr)]

I have taken them into consideration.

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/56646