Bug 44567 – If an <embed> is part of an <object> element's fallback content, WebKit should only render the <embed> if the <object> fails to load.

• Component: New Bugs
• Status: RESOLVED FIXED

[Andy Estes]

If an EMBED is part of an OBJECT's fallback content, WebKit should only render it if the OBJECT fails to load.

[Andy Estes]

In the case where a OBJECT has an EMBED in its fallback content, WebKit has traditionally loaded the EMBED instead of the OBJECT.  For instance, in WebKit:

    <object data="movie.swf">
        <p>Fallback content</p>
        <embed src="othermovie.mov"></embed>
    </object>

was equivalent to writing:

    <embed src="othermovie.mov"></embed>

The presence of a child <embed> causes WebKit to not attempt to load the <object>, even though they reference different resources. Furthermore, WebKit ignores the non-embed fallback content of the <object> that other browsers would render if the <object> fails to load.

This behavior causes WebKit-based browsers to behave differently from other modern browsers (Firefox, Opera and IE all understand both <object> and <embed> at this point), and it diverges from how HTML5 specifies <object> (http://dev.w3.org/html5/spec/Overview.html#the-object-element).

[Andy Estes]

<rdar://problem/7699852>

[Andy Estes]

Created attachment 65357 [details]
Patch

[Andy Estes]

Created attachment 65474 [details]
Patch (v2)

Added additional test coverage.

[Darin Adler]

Comment on attachment 65474 [details]
Patch (v2)

> +        If an EMBED is part of an OBJECT's fallback content, WebKit should only
> +        render the EMBED if the OBJECT fails to load.

Those capitalized tag names are so old school! I think <embed> and <object> are so much more 2010.

>      Node* p = parentNode();
>      if (p && p->hasTagName(objectTag)) {
>          ASSERT(p->renderer());
> -        return false;
> +        if (!static_cast<HTMLObjectElement*>(p)->useFallbackContent())
> +            return false;
>      }

A "why" comment would be so great here.

> +    bool useFallbackContent() const { return m_useFallbackContent; }

My only concern about exposing this is that I don't know when the value is set up. For example, might HTMLEmbedElement::rendererIsNeeded call this function before the <object> has decided whether to use fallback content?

When is it safe to call this? Maybe only when we have made a renderer? Can we add some kind of assertion?

> -                    url = p->value();
> +                    url = deprecatedParseURL(p->value());

This looks like a good bug fix, but perhaps one that is separate from the main change here. Did you add a test case to cover this fix? Should we land this fix separately, first?

> +        // Turn the attributes of the OBJECT tag into arrays, but don't override PARAM values.

I think "OBJECT tag" and "PARAM" should be either "object element" or "<object> element" and "<param> values".

> +        * accessibility/plugin.html: Simplified this test by removing the
> +        unnecessary <object> around the <embed>.

But don't we also want test coverage for the common case where an <object> is around an <embed>?

[Andy Estes]

(In reply to comment #5)
> (From update of attachment 65474 [details])
> > +        If an EMBED is part of an OBJECT's fallback content, WebKit should only
> > +        render the EMBED if the OBJECT fails to load.
> 
> Those capitalized tag names are so old school! I think <embed> and <object> are so much more 2010.

I'll change this to get with the times!

> 
> >      Node* p = parentNode();
> >      if (p && p->hasTagName(objectTag)) {
> >          ASSERT(p->renderer());
> > -        return false;
> > +        if (!static_cast<HTMLObjectElement*>(p)->useFallbackContent())
> > +            return false;
> >      }
> 
> A "why" comment would be so great here.

Will do.

> 
> > +    bool useFallbackContent() const { return m_useFallbackContent; }
> 
> My only concern about exposing this is that I don't know when the value is set up. For example, might HTMLEmbedElement::rendererIsNeeded call this function before the <object> has decided whether to use fallback content?

I don't think so. HTMLObjectElement::renderFallbackContent() sets m_useFallbackContent to true and then calls attach(), which ends up calling HTMLEmbedElement::rendererIsNeeded(). HTMLEmbedElement::rendererIsNeeded() might be called prior to this, but this will be before we've made the decision to render fallback content, so we will not create a renderer for the <embed>, which is correct behavior.

> 
> When is it safe to call this? Maybe only when we have made a renderer? Can we add some kind of assertion?

We can probably assert that the <object> element's renderer is a RenderInline instead of a RenderEmbeddedObject. That'll ensure that m_useFallbackContent wasn't set to true without also tearing down the old renderer and replacing it with a RenderInline.

> 
> > -                    url = p->value();
> > +                    url = deprecatedParseURL(p->value());
> 
> This looks like a good bug fix, but perhaps one that is separate from the main change here. Did you add a test case to cover this fix? Should we land this fix separately, first?

You're right. This change broke a layout test in xssAuditor which exposed this bug, but it's logically separate. I'll submit a patch with this by itself.

> 
> > +        // Turn the attributes of the OBJECT tag into arrays, but don't override PARAM values.
> 
> I think "OBJECT tag" and "PARAM" should be either "object element" or "<object> element" and "<param> values".

Will do.

> 
> > +        * accessibility/plugin.html: Simplified this test by removing the
> > +        unnecessary <object> around the <embed>.
> 
> But don't we also want test coverage for the common case where an <object> is around an <embed>?

I don't know why I did this. The expected results changed so I decided to update the test rather than update the results, but I can do it the other way around.

Thanks for the review!

[Andy Estes]

(In reply to comment #5)
> > -                    url = p->value();
> > +                    url = deprecatedParseURL(p->value());
> 
> This looks like a good bug fix, but perhaps one that is separate from the main change here. Did you add a test case to cover this fix? Should we land this fix separately, first?

Fixed in http://trac.webkit.org/changeset/66137.

[Andy Estes]

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