Bug 36675 - Re-entrant layout via plug-ins may cause crashes with bad RenderWidgets
Summary: Re-entrant layout via plug-ins may cause crashes with bad RenderWidgets
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-03-26 14:18 PDT by Simon Fraser (smfr)
Modified: 2010-03-26 15:26 PDT (History)
3 users (show)

See Also:


Attachments
Patch (11.41 KB, patch)
2010-03-26 14:47 PDT, Simon Fraser (smfr)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2010-03-26 14:18:23 PDT
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().
Comment 1 Simon Fraser (smfr) 2010-03-26 14:18:52 PDT
<rdar://problem/7787617>
Comment 2 Simon Fraser (smfr) 2010-03-26 14:47:10 PDT
Created attachment 51780 [details]
Patch
Comment 3 WebKit Review Bot 2010-03-26 14:49:14 PDT
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.
Comment 4 Geoffrey Garen 2010-03-26 14:51:48 PDT
+    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.
Comment 5 Simon Fraser (smfr) 2010-03-26 14:53:29 PDT
RenderEmbeddedObject is manually ref-counted, not RefCounted, so I can't use RefPtr.
Comment 6 mitz 2010-03-26 14:59:40 PDT
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.
Comment 7 Darin Adler 2010-03-26 15:00:22 PDT
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.
Comment 8 Darin Adler 2010-03-26 15:00:43 PDT
Comment on attachment 51780 [details]
Patch

Oops, meant to leave this as r=mitz.
Comment 9 mitz 2010-03-26 15:11:19 PDT
I agree with Darin’s comments :)
Comment 10 Simon Fraser (smfr) 2010-03-26 15:15:52 PDT
I have taken them into consideration.
Comment 11 Simon Fraser (smfr) 2010-03-26 15:26:44 PDT
http://trac.webkit.org/changeset/56646