Bug 41054 – WebCore crashes when removing an object element with fallback content in a beforeload handler

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Andy Estes]

If an image-based <object> with fallback content is removed from the DOM in a beforeload listener and the image fails to load, WebCore will crash trying to render the fallback content.  Here is the relevant stacktrace:

0   com.apple.WebCore             	0x00000001013fc15d WebCore::Node::createRendererIfNeeded() + 45
1   com.apple.WebCore             	0x0000000100e76d30 WebCore::Element::attach() + 32
2   com.apple.WebCore             	0x0000000100c8d19c WebCore::CachedImage::checkNotify() + 76
3   com.apple.WebCore             	0x0000000100c8da5c WebCore::CachedImage::data(WTF::PassRefPtr<WebCore::SharedBuffer>, bool) + 364
4   com.apple.WebCore             	0x00000001013c1456 WebCore::Loader::Host::didFinishLoading(WebCore::SubresourceLoader*) + 358
5   com.apple.WebCore             	0x00000001015b7cc1 WebCore::SubresourceLoader::didFinishLoading() + 49

The attached .html file demonstrates the crash.

[Andy Estes]

<rdar://problem/8107855>

[Andy Estes]

It looks like the crash happens when removing any <object> with an invalid data URL.  It isn't specific to image URLs or <object>'s with fallback content as I had originally thought.  Patch forthcoming.

[Andy Estes]

Created attachment 59501 [details]
test case

[Andy Estes]

Created attachment 59502 [details]
patch

[Alexey Proskuryakov]

Comment on attachment 59502 [details]
patch

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

[Alexey Proskuryakov]

Comment on attachment 59502 [details]
patch

+    <object data="invalid.png">FAIL</object>
+    <object data="invalid">FAIL</object>

One more trivial comment - it's slightly better to use something like "does-not-exist", because "invalid" implies broken content or URL, while neither is the case here. ChangeLog is also wrong to call the URL invalid.

[Andy Estes]

(In reply to comment #5)
> (From update of attachment 59502 [details])
> > +        (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.

It does.  The crashing behavior is actually slightly different depending on whether or not the data URL has an image extension, so I'm testing both cases in the layout test.

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

By 'destroyed', I mean RenderWidget::destroy() was called.  Among other things, RenderWidget::destroy() calls setNode(0).  So, it hasn't been deleted yet, but it has been destroyed.  Perhaps I can make that more clear in the comment.

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

You're right that the comment is probably overkill; I'll just make it shorter.

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

Actually, dispatchBeforeLoadEvent() will return false if the beforeload JavaScript listener returns false or calls preventDefault().  This is the way the script tells us to block the load.

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

Thanks for the review!

[Andy Estes]

(In reply to comment #7)
> > 
> > > +        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".
> 
> Actually, dispatchBeforeLoadEvent() will return false if the beforeload JavaScript listener returns false or calls preventDefault().  This is the way the script tells us to block the load.

Ohh duh!  Sorry, I totally misunderstood your point.  Yes, 'beforeLoadAllowed' makes more sense.

[Alexey Proskuryakov]

> Actually, dispatchBeforeLoadEvent() will return false if the beforeload JavaScript listener returns false or > calls preventDefault().  This is the way the script tells us to block the load.

Yes, but isn't returning false also a success for dispatchBeforeLoadEvent()? That's why it exists, after all.

[Alexey Proskuryakov]

Sorry, I didn't notice that you already corrected yourself.

[Andy Estes]

Committed http://trac.webkit.org/changeset/61707

[Eric Seidel (no email)]

Looks like fast/dom/beforeload/remove-bad-object-in-beforeload-listener.html was landed w/o results.

[Andy Estes]

(In reply to comment #12)
> Looks like fast/dom/beforeload/remove-bad-object-in-beforeload-listener.html was landed w/o results.

Bah!  Fixing...

[Andy Estes]

(In reply to comment #13)
> (In reply to comment #12)
> > Looks like fast/dom/beforeload/remove-bad-object-in-beforeload-listener.html was landed w/o results.
> 
> Bah!  Fixing...

Committed http://trac.webkit.org/changeset/61721 with layout test expectations.