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
Created attachment 113561 [details] Patch Override isURLAttribute() for HTMLElement and return true if attribute is itemidAttr.
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).
Created attachment 113652 [details] Updated patch Incorporating review comments.
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.
Created attachment 113792 [details] Updated patch Incorporating review comments, also added a test-case for the issue.
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.
(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.
Created attachment 114214 [details] Updated patch Incorporating review comments.
Comment on attachment 114214 [details] Updated patch Clearing flags on attachment: 114214 Committed r99742: <http://trac.webkit.org/changeset/99742>
All reviewed patches have been landed. Closing bug.