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().
<rdar://problem/7787617>
Created attachment 51780 [details] Patch
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.
+ 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.
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.
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 on attachment 51780 [details] Patch Oops, meant to leave this as r=mitz.
I agree with Darin’s comments :)
I have taken them into consideration.
http://trac.webkit.org/changeset/56646