Summary: | Re-entrant layout via plug-ins may cause crashes with bad RenderWidgets | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||
Component: | Layout and Rendering | Assignee: | Simon Fraser (smfr) <simon.fraser> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | ggaren, mitz, webkit.review.bot | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | PC | ||||||
OS: | OS X 10.5 | ||||||
Attachments: |
|
Description
Simon Fraser (smfr)
2010-03-26 14:18:23 PDT
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. |