Bug 54680 – Remove Invalid ASSERT in SubframeLoader::requestObject

• Status: RESOLVED
• Resolution: INVALID
• Component: WebCore Misc.

[Joseph Pecoraro]

SubframeLoader::requestObject:

    // FIXME: None of this code should use renderers!
    RenderEmbeddedObject* renderer = ownerElement->renderEmbeddedObject();
    if (!renderer)
        return false;

renderEmbeddedObject may return 0 when the ownerElement is not an
isEmbeddedObject, which may be the case if has fallback content.
See HTMLPlugInImageElement::createRenderer, and the HTMLObjectElement
subclass.

This ASSERT is unnecessary. Also, there is no change since the condition is
checked immediately afterwards. It can happen when an HTMLObjectElement
(a subclass) is passed in as the HTMLPlugInImageElement parameter. In
release builds this would act correctly and return, in which case the
HTMLObjectElement will render its fallback content (see
HTMLObjectElement::updateWidget).

[Joseph Pecoraro]

Created attachment 82847 [details]
[PATCH] Remove ASSERT

[Joseph Pecoraro]

Haha, I pasted the new code. The old code had an ASSERT! It looked like:

    // FIXME: None of this code should use renderers!
    RenderEmbeddedObject* renderer = ownerElement->renderEmbeddedObject();
    ASSERT(renderer);
    if (!renderer)
        return false;

[Alexey Proskuryakov]

Can this be tested with a regression test? That seems quite important in this case.

[Eric Seidel (no email)]

This reminded me of bug 54711.  (I think I edited these sections of code around the same time.)

[Eric Seidel (no email)]

Comment on attachment 82847 [details]
[PATCH] Remove ASSERT

Please replace the ASSERT with a comment explaining when it can return 0 there.  The ASSERT clearly came from confusion here.

[Joseph Pecoraro]

Actually, this ASSERT would have helped catch situations like the following:
<http://webkit.org/b/47550> For WebKit plug-ins, beforeload can be called recursively (esp. with AdBlock style extensions)

I see now that I was using an older version of WebKit that allowed this type of recursion
in creating Renders during a beforeload event. I think it might be worth keeping
this ASSERT in, but adding a comment to explain that if this ASSERT happens, it
may be another case of an unexpected renderer for an HTMLPluginElement.
Something like:

    // If the render is null, this HTMLPlugInImageElement likely has a fallback renderer
    // Why there is a fallback renderer? Does this it makes sense that there is one?
    ASSERT(renderer);

Worth adding?

I know the path I just explained still exists, but it doesn't seem to happen in practice,
and the case where I saw it was with an outdated WebKit. There is already a FIXME
comment to remove renderers from SubframeLoader::requestObject anyways.

[Darin Adler]

Comment on attachment 82847 [details]
[PATCH] Remove ASSERT

Sounds like Joe changed his mind about this, so removing review+ from it.

[Joseph Pecoraro]

Reason for closing:

> I was using an older version of WebKit that allowed this type of
> recursion in creating Renders during a beforeload event.

But that is no longer the case.