[webkit-reviews] review granted: [Bug 41054] WebCore crashes when removing an object element with fallback content in a beforeload handler : [Attachment 59502] patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Andy Estes <aestes at apple.com>'s
request for review:
Bug 41054: WebCore crashes when removing an object element with fallback
content in a beforeload handler
https://bugs.webkit.org/show_bug.cgi?id=41054

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
> +	   (WebCore::RenderEmbeddedObject::updateWidget): If the RenderObject
was
> +	   destroyed during processing of onbeforeload, do not proceed with
loading
> +	   the object.

So the same regression test covers both fixes? That's great news.

> +	    // may cause this RenderObject to be destroyed.  Even though we
protect

As you explain later, it's not destroyed. I'm not sure what the correct term
is.

> +	   // crashes such as the one described in <rdar://problem/8107855>. 
Exit

It is better to give b.w.o. references, since non-Apple people can't see radar
bugs. Or maybe this comment isn't needed at all, or could be made much shorter
("The node may get detached by event handler"). In retrospect, this seems
pretty obvious.

> +	   bool success = beforeLoadSuccess &&
frame->loader()->subframeLoader()->requestObject(this, url,
objectElement->getAttribute(nameAttr), serviceType, paramNames, paramValues);

I think that dispatchBeforeLoadEvent() always "succeeds", so it would be
slightly better for the variable to be called something like
"beforeLoadAllowedLoad".

r=me assuming that both code changes are covered by the regression test.