Bug 120297 – Need to check if some HTML child elements are HTMLUnknownElement

• Component: DOM
• Status: RESOLVED FIXED

[Gyuyoung Kim]

As mentioned in https://bugs.webkit.org/show_bug.cgi?id=119951#c13, HTMLUnknownElement can own specific tag(e.g. audiotag, videoTag and so on) though it isn't correct element class. In this case, isHTMLXXXElement() returns "true" though it is HTMLUnknowElement.


PassRefPtr<HTMLElement> HTMLElementFactory::createHTMLElement(const QualifiedName& qName, Document* document, HTMLFormElement* formElement, bool createdByParser)
{
    if (!document)
        return 0;

#if ENABLE(CUSTOM_ELEMENTS)
    if (document->registry()) {
        if (RefPtr<CustomElementConstructor> constructor = document->registry()->find(nullQName(), qName)) {
            RefPtr<Element> element = constructor->createElement();
            ASSERT(element->isHTMLElement());
            return static_pointer_cast<HTMLElement>(element.release());
        }
    }
#endif

    if (!gFunctionMap)
        createFunctionMap();
    if (ConstructorFunction function = gFunctionMap->get(qName.localName().impl())) {
        if (PassRefPtr<HTMLElement> element = function(qName, document, formElement, createdByParser))
            return element;
    }

    return HTMLUnknownElement::create(qName, document);

[Gyuyoung Kim]

I wonder if we can add "unknownTag" for HTMLUnknownElement. Does anyone know if we can add it ?

[Zan Dobersek]

I was later thinking of simply checking that, for the problematic elements, the HTMLElement is not a HTMLUnknownElement:

inline bool isHTMLAudioElement(Node* node)
{
    return node->hasTagName(HTMLNames::audioTag) && !toHTMLElement(node)->isHTMLUnknownElement();
}

[Gyuyoung Kim]

(In reply to comment #2)
> I was later thinking of simply checking that, for the problematic elements, the HTMLElement is not a HTMLUnknownElement:
> 
> inline bool isHTMLAudioElement(Node* node)
> {
>     return node->hasTagName(HTMLNames::audioTag) && !toHTMLElement(node)->isHTMLUnknownElement();
> }

Yes, I also think we can fix this issue by using it. However, it that case, we may need to add additional condition to many isHTMLXXXElement() functions. If we can't add unknownTag, it would be alternative solution. :)

[Gyuyoung Kim]

(In reply to comment #3)
> (In reply to comment #2)
> > I was later thinking of simply checking that, for the problematic elements, the HTMLElement is not a HTMLUnknownElement:
> > 
> > inline bool isHTMLAudioElement(Node* node)
> > {
> >     return node->hasTagName(HTMLNames::audioTag) && !toHTMLElement(node)->isHTMLUnknownElement();
> > }
> 
> Yes, I also think we can fix this issue by using it. However, it that case, we may need to add additional condition to many isHTMLXXXElement() functions. If we can't add unknownTag, it would be alternative solution. :)

It looks we can't add "unknown" attribute because it should own a tag which is defined in a page. Even if we define new "unknown" attribute, we make a new attribute without spec definition. So, I think we can fix this issue using adding additional condition to isHTMLXXXElement() functions.

[Zan Dobersek]

The methods that need changing are not that many - only the ones that cast to an element of which the constructor in HTMLElementFactory can return 0.

[Gyuyoung Kim]

(In reply to comment #5)
> The methods that need changing are not that many - only the ones that cast to an element of which the constructor in HTMLElementFactory can return 0.

Yes, right. Even three HTMLElement classes only have isHTMLXXXElement() functions.

 - HTMLSourceElement.h
 - HTMLTrackElement.h
 - HTMLContentElement.h

I don't think we need to add isHTMLXXXElement() to other "return 0" classes which don't have the isHTMLXXXElement() in order to fix this problem at the moment.

[Gyuyoung Kim]

Created attachment 209654 [details]
Patch without test case

[Gyuyoung Kim]

Let me try to add a test tomorrow.

[Zan Dobersek]

I've posted one test case (for the audio element) back in bug #119951.

[Gyuyoung Kim]

(In reply to comment #9)
> I've posted one test case (for the audio element) back in bug #119951.

If you don't mind, can I use it ? I will add your name to ChangeLog as well.

[Zan Dobersek]

(In reply to comment #10)
> (In reply to comment #9)
> > I've posted one test case (for the audio element) back in bug #119951.
> 
> If you don't mind, can I use it ? I will add your name to ChangeLog as well.

Sure.

Test cases for other elements (source, track) should work pretty much the same. I haven't looked into how the crash with HTMLContentElement could be produced.

[Geoffrey Garen]

There are 1305 uses of hasTagName() in WebKit. Are all of them wrong, since they might accidentally be true for an HTMLUnknownElement? What patch introduced this behavior?

[Geoffrey Garen]

Patching this in just one or two places is probably the wrong solution, since it doesn't address the fundamental problem that any line of code that tests for tag name might allow an HTMLUnknownElement through.

[Chris Dumez]

Comment on attachment 209654 [details]
Patch without test case

View in context: https://bugs.webkit.org/attachment.cgi?id=209654&action=review

> Source/WebCore/html/HTMLSourceElement.h:-67
> -    return element->hasTagName(HTMLNames::sourceTag);

We can only get an HTMLUnknownElement for sourceTag if VIDEO is disabled at compile time. Why not simply do:
#if (VIDEO)
    return element->hasTagName(HTMLNames::sourceTag)
#else
    UNUSED_PARAM(element);
    return false;
#endif

> Source/WebCore/html/HTMLTrackElement.h:102
> +    return node->hasTagName(HTMLNames::trackTag) && !toHTMLElement(node)->isHTMLUnknownElement();

Ditto with VIDEO_TRACK flag

> Source/WebCore/html/HTMLTrackElement.h:107
> +    return element->hasTagName(HTMLNames::trackTag) && !toHTMLElement(element)->isHTMLUnknownElement();

Ditto

[Chris Dumez]

Comment on attachment 209654 [details]
Patch without test case

View in context: https://bugs.webkit.org/attachment.cgi?id=209654&action=review

>> Source/WebCore/html/HTMLSourceElement.h:-67
>> -    return element->hasTagName(HTMLNames::sourceTag);
> 
> We can only get an HTMLUnknownElement for sourceTag if VIDEO is disabled at compile time. Why not simply do:
> #if (VIDEO)
>     return element->hasTagName(HTMLNames::sourceTag)
> #else
>     UNUSED_PARAM(element);
>     return false;
> #endif

BTW, this looks like a good reason to generate those isXXXElement functions. It is bug prone otherwise.

[Gyuyoung Kim]

(In reply to comment #13)
> Patching this in just one or two places is probably the wrong solution, since it doesn't address the fundamental problem that any line of code that tests for tag name might allow an HTMLUnknownElement through.

I wanted to solve this issue by fixing root cause. So, I thought that HTMLUnknownElement() needs to have own tag name in order to do fix it. However, it looks it is spec violation. In this case, problem is that HTMLUnknownElement can have valid element. (It only can have audioTag, sourceTag, trackTag, videoTag, contentElement and dialogTag.)

Then, how about setting qName as null as below ?, or do you think we can define new tag for this case ?

if (ConstructorFunction function = gFunctionMap->get(qName.localName().impl())) {
    if (PassRefPtr<HTMLElement> element = function(qName, document, formElement, createdByParser))
        return element;
    else
        qName = 0 or new tag;
}

return HTMLUnknownElement::create(qName, document);

[Gyuyoung Kim]

Comment on attachment 209654 [details]
Patch without test case

View in context: https://bugs.webkit.org/attachment.cgi?id=209654&action=review

>>> Source/WebCore/html/HTMLSourceElement.h:-67
>>> -    return element->hasTagName(HTMLNames::sourceTag);
>> 
>> We can only get an HTMLUnknownElement for sourceTag if VIDEO is disabled at compile time. Why not simply do:
>> #if (VIDEO)
>>     return element->hasTagName(HTMLNames::sourceTag)
>> #else
>>     UNUSED_PARAM(element);
>>     return false;
>> #endif
> 
> BTW, this looks like a good reason to generate those isXXXElement functions. It is bug prone otherwise.

We need to consider HTMLElementFactory return 0 with a tag. However, it looks some ctor functions can return 0 when the macro is enabled at compile time. So, It don't think we can avoid this problem when using ENABLE() macro.

In WebKitBuild/Release/DerivedSources/WebCore/HTMLElementFactory.cpp

#if ENABLE(SHADOW_DOM)

static PassRefPtr<HTMLElement> contentConstructor(const QualifiedName& tagName, Document* document, HTMLFormElement*, bool)
{
    if (!RuntimeEnabledFeatures::shadowDOMEnabled())
        return 0;
    return HTMLContentElement::create(tagName, document);
}

#endif

#if ENABLE(DIALOG_ELEMENT)

static PassRefPtr<HTMLElement> dialogConstructor(const QualifiedName& tagName, Document* document, HTMLFormElement*, bool)
{
    if (!ContextFeatures::dialogElementEnabled(document))
        return 0;
    return HTMLDialogElement::create(tagName, document);
}

#endif

#if ENABLE(VIDEO)

static PassRefPtr<HTMLElement> sourceConstructor(const QualifiedName& tagName, Document* document, HTMLFormElement*, bool)
{
    Settings* settings = document->settings();
    if (!MediaPlayer::isAvailable() || (settings && !settings->mediaEnabled()))
        return 0;

    return HTMLSourceElement::create(tagName, document);
}

#endif

...

[Gyuyoung Kim]

(In reply to comment #16)
 
> Then, how about setting qName as null as below ?, or do you think we can define new tag for this case ?
> 
> if (ConstructorFunction function = gFunctionMap->get(qName.localName().impl())) {
>     if (PassRefPtr<HTMLElement> element = function(qName, document, formElement, createdByParser))
>         return element;
>     else
>         qName = 0 or new tag;
> }
> 
> return HTMLUnknownElement::create(qName, document);

qName can't be changed because it is const. So, we may try to return HTMLUnknownElement with a null QualifiedName() as below,

    else
        return HTMLUnknownElement::create(QualifiedName(nullAtom, nullAtom, nullAtom), document);


    return HTMLUnknownElement::create(qName, document);

[Chris Dumez]

(In reply to comment #17)
> (From update of attachment 209654 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209654&action=review
> 
> >>> Source/WebCore/html/HTMLSourceElement.h:-67
> >>> -    return element->hasTagName(HTMLNames::sourceTag);
> >> 
> >> We can only get an HTMLUnknownElement for sourceTag if VIDEO is disabled at compile time. Why not simply do:
> >> #if (VIDEO)
> >>     return element->hasTagName(HTMLNames::sourceTag)
> >> #else
> >>     UNUSED_PARAM(element);
> >>     return false;
> >> #endif
> > 
> > BTW, this looks like a good reason to generate those isXXXElement functions. It is bug prone otherwise.
> 
> We need to consider HTMLElementFactory return 0 with a tag. However, it looks some ctor functions can return 0 when the macro is enabled at compile time. So, It don't think we can avoid this problem when using ENABLE() macro.
> 
> In WebKitBuild/Release/DerivedSources/WebCore/HTMLElementFactory.cpp
> 
> #if ENABLE(SHADOW_DOM)
> 
> static PassRefPtr<HTMLElement> contentConstructor(const QualifiedName& tagName, Document* document, HTMLFormElement*, bool)
> {
>     if (!RuntimeEnabledFeatures::shadowDOMEnabled())
>         return 0;
>     return HTMLContentElement::create(tagName, document);
> }
> 
> #endif
> 
> #if ENABLE(DIALOG_ELEMENT)
> 
> static PassRefPtr<HTMLElement> dialogConstructor(const QualifiedName& tagName, Document* document, HTMLFormElement*, bool)
> {
>     if (!ContextFeatures::dialogElementEnabled(document))
>         return 0;
>     return HTMLDialogElement::create(tagName, document);
> }
> 
> #endif
> 
> #if ENABLE(VIDEO)
> 
> static PassRefPtr<HTMLElement> sourceConstructor(const QualifiedName& tagName, Document* document, HTMLFormElement*, bool)
> {
>     Settings* settings = document->settings();
>     if (!MediaPlayer::isAvailable() || (settings && !settings->mediaEnabled()))
>         return 0;
> 
>     return HTMLSourceElement::create(tagName, document);
> }
> 
> #endif
> 
> ...

Right, is some cases the feature is enabled at runtime, not simply compile time. However, since this factory is generated, this means we already have all the information we need to generate the isHTML*Element() functions, rignt?

[Gyuyoung Kim]

(In reply to comment #19)
> 
> Right, is some cases the feature is enabled at runtime, not simply compile time. However, since this factory is generated, this means we already have all the information we need to generate the isHTML*Element() functions, rignt?

I'm not sure if we should generate isHTML*Element() for all tags at the moment. However, I think we need to support isHTML*Element() for 'return 0' ctor at least.

[Chris Dumez]

(In reply to comment #20)
> (In reply to comment #19)
> > 
> > Right, is some cases the feature is enabled at runtime, not simply compile time. However, since this factory is generated, this means we already have all the information we need to generate the isHTML*Element() functions, rignt?
> 
> I'm not sure if we should generate isHTML*Element() for all tags at the moment. However, I think we need to support isHTML*Element() for 'return 0' ctor at least.

Can you explain why you don't want to generate them?

From what I can see, the isHTML*Element() functions code needs to match what is in an already generated file (the HTMLElementFactory). Since we are able to generate the HTMLElementFactory already, it should be trivial to generate the isHTML*Element() functions. However, doing this work manually seems risky, bug prone and difficult to maintain.

[Gyuyoung Kim]

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > 
> > > Right, is some cases the feature is enabled at runtime, not simply compile time. However, since this factory is generated, this means we already have all the information we need to generate the isHTML*Element() functions, rignt?
> > 
> > I'm not sure if we should generate isHTML*Element() for all tags at the moment. However, I think we need to support isHTML*Element() for 'return 0' ctor at least.
> 
> Can you explain why you don't want to generate them?

There was concerns when we contributed isFooElement() to blink.

One is some C++ Element subclasses represent multiple tag names. e.g. HTMLQuoteElement represents <blockquote> and <q>, however, hasTagName(blockquoteTag) and hasTagName(qTag) are not always checked together. We can't replace all of hasTagName(blockquoteTag) with isHTMLQuoteElement()

Other is to add "#include "FooElement.h" cause unnecessary dependency. HTML parser should avoid to depend on C++ element implementation class if possible.

[Chris Dumez]

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > (In reply to comment #19)
> > > > 
> > > > Right, is some cases the feature is enabled at runtime, not simply compile time. However, since this factory is generated, this means we already have all the information we need to generate the isHTML*Element() functions, rignt?
> > > 
> > > I'm not sure if we should generate isHTML*Element() for all tags at the moment. However, I think we need to support isHTML*Element() for 'return 0' ctor at least.
> > 
> > Can you explain why you don't want to generate them?
> 
> There was concerns when we contributed isFooElement() to blink.
> 
> One is some C++ Element subclasses represent multiple tag names. e.g. HTMLQuoteElement represents <blockquote> and <q>, however, hasTagName(blockquoteTag) and hasTagName(qTag) are not always checked together. We can't replace all of hasTagName(blockquoteTag) with isHTMLQuoteElement()

This seems like an argument not to use the isHTML*Element() in some cases, not an argument against generating the isHTML*Element() functions. I mean, does this statement ever affect the code you write for isHTML*Element()?

> Other is to add "#include "FooElement.h" cause unnecessary dependency. HTML parser should avoid to depend on C++ element implementation class if possible.

Actually, if you generate those isHTML*element() functions, they will likely all be in the same file. e.g. HTMLElementHelpers.h or something like that. And as far as I can see, this file would not need to include any FooElement.h, only HTMLNames.h and likely RuntimeEnabledFeatures.h.

And again, if we don't want to add include in one place, well, we don't have to use the generated isHTML*Element() functions. The fact that they are generated and available does not mandate that you need to use them everywhere.

I am adding a few people in CC as I am curious what WebKit people think about generating these.

[Zan Dobersek]

I'm all for generating the is*Element() and to*Element() functions. Since there are elements for which these helpers are not required, perhaps we could specifically annotate the elements in HTMLTagNames.in that should have them generated.

I guess similar could be done for the SVG elements, in svgtags.in.

[Gyuyoung Kim]

(In reply to comment #24)
> I'm all for generating the is*Element() and to*Element() functions. Since there are elements for which these helpers are not required, perhaps we could specifically annotate the elements in HTMLTagNames.in that should have them generated.

If we can specify which element needs to have isFooElement() or toFooElement(), we may generate the needed isFooElement() and toFooElement().
 
> I guess similar could be done for the SVG elements, in svgtags.in.

If we can adjust the generation into HTML elements, IMHO, we can do it on SVG as well.

[Gyuyoung Kim]

Created attachment 209726 [details]
WIP suggestion according to comment #18

[Gyuyoung Kim]

(In reply to comment #26)
> Created an attachment (id=209726) [details]
> WIP suggestion according to comment #18

I would like to know how do you think to use this suggestion instead of modifying isHTML*Element().

[Chris Dumez]

Comment on attachment 209726 [details]
WIP suggestion according to comment #18

View in context: https://bugs.webkit.org/attachment.cgi?id=209726&action=review

> Source/WebCore/dom/make_names.pl:955
> +            return $parameters{fallbackInterfaceName}::create(QualifiedName(nullAtom, nullAtom, nullAtom), document);

what does the factory code look like with this change? It is not obvious for me.

[Gyuyoung Kim]

(In reply to comment #28)
> (From update of attachment 209726 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209726&action=review
> 
> > Source/WebCore/dom/make_names.pl:955
> > +            return $parameters{fallbackInterfaceName}::create(QualifiedName(nullAtom, nullAtom, nullAtom), document);
> 
> what does the factory code look like with this change? It is not obvious for me.

In HTMLElementFactory::createHTMLElement(), the factory function creates new element. However, as I said many times, some creation functions can return 0. In that case, element is null. then the factory function returns a HTMLUnknownElement instance with valid tags(e.g. audioTag, videoTag, trackTag and so on)

    if (ConstructorFunction function = gFunctionMap->get(qName.localName().impl())) {
        if (PassRefPtr<HTMLElement> element = function(qName, document, formElement, createdByParser))
            return element;
    }

    return HTMLUnknownElement::create(qName, document);

// audioConstructor
static PassRefPtr<HTMLElement> audioConstructor(const QualifiedName& tagName, Document* document, HTMLFormElement*, bool createdByParser)             
{
    Settings* settings = document->settings();
    if (!MediaPlayer::isAvailable() || (settings && !settings->mediaEnabled()))
        return 0; // audioConstructor may return 0.


My second suggestion is that we return HTMLUnknownElement with null tag name because this isn't an unknown situation. This case is that element creation is failed. So, my suggestion is to consider to return HTMLUnknownElement() with null tag.

As far as I understand, HTMLUnknownElement supports tags out of specification, for example, <foo1></foo1>, <stupid></stupid> and so on. But, this case is not the situation for it.

[Zan Dobersek]

Bug #120584 is working on generating the is*Element() checks. I think we should work on top of that.

[Gyuyoung Kim]

(In reply to comment #30)
> Bug #120584 is working on generating the is*Element() checks. I think we should work on top of that.

ok, let's work on this after landing it. BTW, current test case doesn't make a crash on EFL port. I'm taking a look it.

[Xueqing Huang]

(In reply to comment #31)
> (In reply to comment #30)
> > Bug #120584 is working on generating the is*Element() checks. I think we should work on top of that.
> 
> ok, let's work on this after landing it. BTW, current test case doesn't make a crash on EFL port. I'm taking a look it.

Windows port could reproduce the crash caused by this issue.
HTMLUnknownElement override |isHTMLUnknownElement| and return true, I suggest check whether an element was a HTMLUnknownElement or not via isHTMLUnknownElement in |isHTML*Element()|. e.g:
bool isHTMLAudioElement(const Element& element) { 
    return element.hasTagName(HTMLNames::audioTag) && !isHTMLUnknownElement(element);
}

[Xueqing Huang]

*** Bug 121537 has been marked as a duplicate of this bug. ***

[Darin Adler]

(In reply to comment #2)
> I was later thinking of simply checking that, for the problematic elements, the HTMLElement is not a HTMLUnknownElement:
> 
> inline bool isHTMLAudioElement(Node* node)
> {
>     return node->hasTagName(HTMLNames::audioTag) && !toHTMLElement(node)->isHTMLUnknownElement();
> }

Yes, I think this is exactly what we want to do, only for the elements that can be affected by this.

[Darin Adler]

(In reply to comment #29)
> However, as I said many times, some creation functions can return 0. In that case, element is null.

Exactly. We will need code to check for unknown elements in the isXXX function for any of the creation functions that can return nullptr.

And we should minimize our use of this technique. It’s not really a good pattern.

[Gyuyoung Kim]

Hmm, crash doesn't occurs on existing test. It looks this crash was fixed indirectly. I will check if this crash is really fixed. Can anyone reproduce this crash now ?

[Darin Adler]

I don’t see evidence that the isHTMLAudioElement function has been fixed. The script that generates constructors and isHTMLAudioElement functions still seems to be checking MediaPlayer::isAvailable and settings && !settings->mediaEnabled() in the audio element constructor and in the audio element createWrapper function, but not in the isHTMLAudioElement function. From what I see from code inspection, this still needs to be fixed.

[Zan Dobersek]

I can still reproduce this crash on the GTK port with running the test case I've attached in bug #119951 through either DumpRenderTree or WebKitTestRunner. Things basically go FUBAR when WebCore::JSNodeOwner::isReachableFromOpaqueRoots is reached during the GC and the isHTMLAudioElement() check returns a false positive in the isReachableFromDOM function.

[Gyuyoung Kim]

Created attachment 213353 [details]
WIP

[Gyuyoung Kim]

First of all, sorry for too late update. BTW, I'm suspecting if we need to return a fallback element when MediaPlayer isn't available. Besides there was a similar patch for V8 binding.

https://bugs.webkit.org/show_bug.cgi?id=103431

Unfortunately, I have a reproduce problem because of my env problem. I will re-submit a new patch after fixing my env problem.

[Darin Adler]

I’ll fix this.

[Darin Adler]

Created attachment 213445 [details]
Patch

[WebKit Commit Bot]

Attachment 213445 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/dom/make_names.pl', u'Source/WebCore/html/HTMLElement.h']" exit_code: 1
Source/WebCore/html/HTMLElement.h:147:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

Comment on attachment 213445 [details]
Patch

r=me
The previous patch on this bug had a layout test. We should make sure to add it as well, assuming it works.

[Gyuyoung Kim]

Darin, please land after fixing style error.

[Darin Adler]

(In reply to comment #45)
> Darin, please land after fixing style error.

That’s not a style error. That’s the style script incorrectly complaining about an include that is not not part of the normal includes at the top of the file.

[Darin Adler]

(In reply to comment #44)
> The previous patch on this bug had a layout test. We should make sure to add it as well, assuming it works.

I’ll add the layout test and check that it demonstrates the bug before I land.

[Darin Adler]

Committed r156953: <http://trac.webkit.org/changeset/156953>