Bug 71510 – [Microdata] LayoutTests/fast/dom/MicroData/itemid-attribute-test.html assertion failure in Element::getURLAttribute()

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Arko Saha]

We are hitting an assertion failure while retriving itemid of a Microdata item, as isURLAttribute() returns false.

ASSERTION FAILED: isURLAttribute(attribute)

Backtrace:
#0  0x00810424 in WebCore::Element::getURLAttribute (this=0x82512f8, name=...) at ../../Source/WebCore/dom/Element.cpp:1928
#1  0x0118bf15 in WebCore::jsHTMLElementItemId (exec=0xb2fbe0f8, slotBase=...) at DerivedSources/WebCore/JSHTMLElement.cpp:430
#2  0x03d2367d in JSC::PropertySlot::getValue (this=0xbfffdad8, exec=0xb2fbe0f8, propertyName=...)
    at ../../Source/JavaScriptCore/runtime/PropertySlot.h:75
#3  0x03dbbc3d in JSC::JSValue::get (this=0xbfffdb0c, exec=0xb2fbe0f8, propertyName=..., slot=...)
    at ../../Source/JavaScriptCore/runtime/JSObject.h:799
#4  0x03df54d9 in JSC::cti_op_get_by_id (args=0xbfffdb50) at ../../Source/JavaScriptCore/jit/JITStubs.cpp:1654
#5  0x03df324a in JSC::JITThunks::tryCacheGetByID (callFrame=0xb2fb2930, codeBlock=0xfffffffb, returnAddress=..., baseValue=..., 
    propertyName=..., slot=..., stubInfo=0xbfffdb88) at ../../Source/JavaScriptCore/jit/JITStubs.cpp:937
#6  0xbfffdb88 in ?? ()
#7  0x03dbc27d in JSC::JITCode::execute (this=0xb2f97e40, registerFile=0x815c43c, callFrame=0xb2fbe0f8, globalData=0x80f0000)
    at ../../Source/JavaScriptCore/jit/JITCode.h:103
#8  0x03dba2ed in JSC::Interpreter::execute (this=0x815c430, eval=0xb2f97e30, callFrame=0xb2fbe088, thisValue=..., globalRegisterOffset=31, 
    scopeChain=0xb2f9ef50) at ../../Source/JavaScriptCore/interpreter/Interpreter.cpp:1295
#9  0x03db6227 in JSC::Interpreter::callEval (this=0x815c430, callFrame=0xb2fbe088, registerFile=0x815c43c, argv=0xb2fbe0b8, argc=2, 
    registerOffset=14) at ../../Source/JavaScriptCore/interpreter/Interpreter.cpp:463
#10 0x03dfddc8 in JSC::cti_op_call_eval (args=0xbfffe200) at ../../Source/JavaScriptCore/jit/JITStubs.cpp:3440
#11 0x03df324a in JSC::JITThunks::tryCacheGetByID (callFrame=0xb2fb37b0, codeBlock=0xfffffffb, returnAddress=..., baseValue=..., 
    propertyName=..., slot=..., stubInfo=0xbfffe238) at ../../Source/JavaScriptCore/jit/JITStubs.cpp:937
#12 0xbfffe238 in ?? ()
#13 0x03dbc27d in JSC::JITCode::execute (this=0xb2f97ea0, registerFile=0x815c43c, callFrame=0xb2fbe038, globalData=0x80f0000)
    at ../../Source/JavaScriptCore/jit/JITCode.h:103
#14 0x03db83ae in JSC::Interpreter::execute (this=0x815c430, program=0xb2f97e90, callFrame=0xb2fa7d24, scopeChain=0xb2f9ffd0, thisObj=
    0xb2fb3fb0) at ../../Source/JavaScriptCore/interpreter/Interpreter.cpp:897
#15 0x03e60a87 in JSC::evaluate (exec=0xb2fa7d24, scopeChain=0xb2f9ffd0, source=..., thisValue=..., returnedException=0xbfffe95c)
    at ../../Source/JavaScriptCore/runtime/Completion.cpp:70
#16 0x0061954d in WebCore::JSMainThreadExecState::evaluate (exec=0xb2fa7d24, chain=0xb2f9ffd0, source=..., thisValue=..., 
    exception=0xbfffe95c) at ../../Source/WebCore/bindings/js/JSMainThreadExecState.h:58
#17 0x0064eaa1 in WebCore::ScriptController::evaluateInWorld (this=0x80daea4, sourceCode=..., world=0x81a6d90)
    at ../../Source/WebCore/bindings/js/ScriptController.cpp:145
#18 0x0064eb9a in WebCore::ScriptController::evaluate (this=0x80daea4, sourceCode=...)
    at ../../Source/WebCore/bindings/js/ScriptController.cpp:162
#19 0x00859bb6 in WebCore::ScriptElement::executeScript (this=0x8210028, sourceCode=...) at ../../Source/WebCore/dom/ScriptElement.cpp:301

[Arko Saha]

Created attachment 113561 [details]
Patch

Override isURLAttribute() for HTMLElement and return true if attribute is itemidAttr.

[Darin Adler]

Comment on attachment 113561 [details]
Patch

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

> Source/WebCore/html/HTMLElement.cpp:997
> +bool HTMLElement::isURLAttribute(Attribute *attr) const

Formatting here is wrong. The * goes next to the word “Attribute”.

In new code we should not use abbreviations. This should be "attribute" instead of "attr".

Should this be unconditional? I don’t think this is right if ENABLE(MICRODATA) is not set.

> Source/WebCore/html/HTMLElement.h:128
> +    virtual bool isURLAttribute(Attribute*) const;

All the classes derived from HTMLElement that currently do not call through to the base class need to do so, otherwise they will return false for itemid. That includes HTMLAnchorElement::isURLAttribute, HTMLBaseElement::isURLAttribute, HTMLBodyElement::isURLAttribute, HTMLButtonElement::isURLAttribute, HTMLEmbedElement::isURLAttribute, HTMLFormElement::isURLAttribute, HTMLFrameElementBase::isURLAttribute, HTMLHtmlElement::isURLAttribute, HTMLIFrameElement::isURLAttribute (actually, that function should just be removed, because HTMLFrameElementBase already takes care of it), HTMLImageElement::isURLAttribute, HTMLInputElement::isURLAttribute, HTMLLinkElement::isURLAttribute, HTMLMediaElement::isURLAttribute, HTMLModElement::isURLAttribute, HTMLObjectElement::isURLAttribute, HTMLParamElement::isURLAttribute, HTMLQuoteElement::isURLAttribute, HTMLScriptElement::isURLAttribute, HTMLSourceElement::isURLAttribute, HTMLTableCellElement::isURLAttribute, HTMLTableElement::isURLAttribute, and HTMLTrackElement::isURLAttribute.

I also wonder if we need this itemid support for SVG elements too (separate issue).

[Arko Saha]

Created attachment 113652 [details]
Updated patch

Incorporating review comments.

[Darin Adler]

Comment on attachment 113652 [details]
Updated patch

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

This is the wrong way to do it. We don’t want to put an #if into all these places that are not microdata-related.

And we need to add a new test that covers all these different types of elements. The existing test does not cover enough.

> Source/WebCore/html/HTMLAnchorElement.cpp:244
> +#if ENABLE(MICRODATA)
> +    return attr->name() == hrefAttr || attr->name() == itemidAttr;
> +#else
>      return attr->name() == hrefAttr;
> +#endif

This code should say:

    return attr->name() == hrefAttr || HTMLElement::isURLAttribute(attr);

Since HTMLAnchorElement’s immediate base class is HTMLElement.

No #if.

Same for all the other functions, but please make sure to use the appropriate base class and not just call straight to HTMLElement each time.

[Arko Saha]

Created attachment 113792 [details]
Updated patch

Incorporating review comments, also added a test-case for the issue.

[Darin Adler]

Comment on attachment 113792 [details]
Updated patch

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

> Source/WebCore/html/HTMLElement.cpp:1004
> +bool HTMLElement::isURLAttribute(Attribute* attribute) const
> +{
> +#if ENABLE(MICRODATA)
> +    return attribute->name() == itemidAttr;
> +#else
> +    return false;
> +#endif
> +}

This will fail to compile on platforms where we have MICRODATA off and we have unused argument warnings on.

> Source/WebCore/html/HTMLElement.h:79
> +    virtual bool isURLAttribute(Attribute*) const;

This should be protected, not public.

> Source/WebCore/html/HTMLInputElement.cpp:1295
> +    return (attr->name() == srcAttr || attr->name() == formactionAttr || HTMLTextFormControlElement::isURLAttribute(attr));

Should remove the unneeded parentheses here.

> Source/WebCore/html/HTMLObjectElement.cpp:387
> +    return (attr->name() == dataAttr || (attr->name() == usemapAttr && attr->value().string()[0] != '#') || HTMLPlugInImageElement::isURLAttribute(attr));

Should remove the unneeded parentheses here.

[Arko Saha]

(In reply to comment #6)
> (From update of attachment 113792 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113792&action=review
> 
> > Source/WebCore/html/HTMLElement.cpp:1004
> > +bool HTMLElement::isURLAttribute(Attribute* attribute) const
> > +{
> > +#if ENABLE(MICRODATA)
> > +    return attribute->name() == itemidAttr;
> > +#else
> > +    return false;
> > +#endif
> > +}
> 
> This will fail to compile on platforms where we have MICRODATA off and we have unused argument warnings on.

Done.
 
> > Source/WebCore/html/HTMLElement.h:79
> > +    virtual bool isURLAttribute(Attribute*) const;
> 
> This should be protected, not public.

Done.

> > Source/WebCore/html/HTMLInputElement.cpp:1295
> > +    return (attr->name() == srcAttr || attr->name() == formactionAttr || HTMLTextFormControlElement::isURLAttribute(attr));
> 
> Should remove the unneeded parentheses here.

Done.
 
> > Source/WebCore/html/HTMLObjectElement.cpp:387
> > +    return (attr->name() == dataAttr || (attr->name() == usemapAttr && attr->value().string()[0] != '#') || HTMLPlugInImageElement::isURLAttribute(attr));
> 
> Should remove the unneeded parentheses here.

Done.

[Arko Saha]

Created attachment 114214 [details]
Updated patch

Incorporating review comments.

[WebKit Review Bot]

Comment on attachment 114214 [details]
Updated patch

Clearing flags on attachment: 114214

Committed r99742: <http://trac.webkit.org/changeset/99742>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.