On Chromium bots we recorded these crashes: media/track/track-css-cue-lifetime.html media/track/track-css-matching.html media/track/track-css-property-whitelist.html http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=@ToT - chromium.org&tests=media/track/track-css-cue-lifetime.html,media/track/track-css-matching.html,media/track/track-css-property-whitelist.html Log points to r140231. Crash log: crash log for DumpRenderTree (pid 14860): STDOUT: <empty> STDERR: ASSERTION FAILED: !element || element->isHTMLUnknownElement() STDERR: Backtrace: STDERR: std::_Init_locks::operator= [0x5FC49F63+172035] STDERR: std::_Init_locks::operator= [0x5FC49EEA+171914] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5F29AB88+15414024] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5F22C79E+14962462] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5E52BA62+1327586] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5E52AE50+1324496] STDERR: v8::Locker::StopPreemption [0x6BF16357+811159] STDERR: v8::Locker::StopPreemption [0x6BF18D6A+821930] STDERR: v8::Locker::StopPreemption [0x6C163D0C+3226188] STDERR: v8::Locker::StopPreemption [0x6C169EF1+3251249] STDERR: (No symbol) [0x1C80A3F6] STDERR: (No symbol) [0x1C83C53B] STDERR: (No symbol) [0x1C80E581] STDERR: (No symbol) [0x1C83B624] STDERR: (No symbol) [0x1C822679] STDERR: (No symbol) [0x1C8134CA] STDERR: v8::Locker::StopPreemption [0x6BEB9B05+432197] STDERR: v8::Locker::StopPreemption [0x6BEB98B4+431604] STDERR: v8::Function::Call [0x6BE33334+516] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5EF43273+11909619] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5EF42FF7+11908983] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5F68CB3F+19550911] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5F69A3F2+19606386] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5F69A0F0+19605616] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5E99D49F+5987359] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5E99CDDF+5985631] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5E9CC0AB+6178859] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5EB21217+7575959] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5EAEC198+7358744] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5EAEB8AC+7356460] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5EAE929C+7346716] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5EAEA5FC+7351676] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5E9CC323+6179491] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5EB73503+7912579] STDERR: std::_Init_locks::operator= [0x5FDF6E88+1929000] STDERR: std::_Init_locks::operator= [0x5FDF7033+1929427] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5EE52233+10922419] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5EE52146+10922182] STDERR: WebDropData::operator= [0x69E635FB+214387] STDERR: WebDropData::operator= [0x69E712CB+270915] STDERR: WebDropData::operator= [0x69E7121A+270738] STDERR: WebDropData::operator= [0x69E7113A+270514] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D1DE35F+451578] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D292D6B+1191430] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D292A96+1190705] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D29350B+1193382] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D2934BA+1193301] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D29341A+1193141] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D1DE35F+451578] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D1E9974+498191] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D1E9DA4+499263] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D1EAC63+503038] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D1F9444+562399] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D1F8552+558573] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D18660C+91815] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D1E94B9+496980] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D1E920B+496294] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D1B4CE9+281988] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D1E8501+492956] STDERR: (No symbol) [0x00D61A4D] STDERR: (No symbol) [0x00C7E45D] STDERR: (No symbol) [0x00CC6D38]
Committed r140450: <http://trac.webkit.org/changeset/140450>
Sorry that was a bad trace from Win.. Here's a trace on OSX 10.7 dbg: crash log for DumpRenderTree (pid 2583): STDOUT: <empty> STDERR: objc[2583]: Class MockCrApp is implemented in both /Volumes/data/b/build/slave/WebKit_Mac10_7__dbg_/build/src/xcodebuild/Debug/libwebkit.dylib and /Volumes/data/b/build/slave/WebKit_Mac10_7__dbg_/build/src/xcodebuild/Debug/DumpRenderTree.app/Contents/MacOS/DumpRenderTree. One of the two will be used. Which one is undefined. STDERR: ASSERTION FAILED: !element || element->isHTMLUnknownElement() STDERR: ../../third_party/WebKit/Source/WebCore/html/HTMLUnknownElement.h(57) : WebCore::HTMLUnknownElement *WebCore::toHTMLUnknownElement(WebCore::HTMLElement *) STDERR: 1 0x6f1403f WebCore::toHTMLUnknownElement(WebCore::HTMLElement*) STDERR: 2 0x6f11449 WebCore::createV8HTMLWrapper(WebCore::HTMLElement*, v8::Handle<v8::Object>, v8::Isolate*) STDERR: 3 0x7a82c14 WebCore::wrap(WebCore::HTMLElement*, v8::Handle<v8::Object>, v8::Isolate*) STDERR: 4 0x7a9f2d3 WebCore::wrap(WebCore::Node*, v8::Handle<v8::Object>, v8::Isolate*) STDERR: 5 0x6e46e64 v8::Handle<v8::Value> WebCore::toV8Fast<v8::AccessorInfo, WebCore::Node>(WebCore::Node*, v8::AccessorInfo const&, WebCore::Node*) STDERR: 6 0x6e26bd8 _ZN7WebCore14NodeV8InternalL20firstChildAttrGetterEN2v85LocalINS1_6StringEEERKNS1_12AccessorInfoE STDERR: 7 0x110efb0 v8::internal::JSObject::GetPropertyWithCallback(v8::internal::Object*, v8::internal::Object*, v8::internal::String*) STDERR: 8 0x110eaa9 v8::internal::Object::GetProperty(v8::internal::Object*, v8::internal::LookupResult*, v8::internal::String*, PropertyAttributes*) STDERR: 9 0x1050128 v8::internal::LoadIC::Load(v8::internal::InlineCacheState, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::String>) STDERR: 10 0x105803e v8::internal::LoadIC_Miss(v8::internal::Arguments, v8::internal::Isolate*) STDERR: 11 0x34a0a3f6 STDERR: Received signal 11 SEGV_MAPERR 0000bbadbeef STDERR: [0x000003d391af] STDERR: [0x000003d3914b] STDERR: [0x000003d38ddb] STDERR: [0x000096f1059b] STDERR: [0x0000ffffffff] STDERR: [0x000006f11449] STDERR: [0x000007a82c14] STDERR: [0x000007a9f2d3] STDERR: [0x000006e46e64] STDERR: [0x000006e26bd8] STDERR: [0x00000110efb0] STDERR: [0x00000110eaa9] STDERR: [0x000001050128] STDERR: [0x00000105803e] STDERR: [0x000034a0a3f6] STDERR: ax: bbadbeef, bx: 6fb2dd00, cx: 6fb2dde5, dx: 6fb2dde5 STDERR: di: 880b1f2, si: 880b19f, bp: c00a67b8, sp: c00a6780, ss: 23, flags: 10286 STDERR: ip: 6f14049, cs: 1b, ds: 23, es: 23, fs: 0, gs: f
(In reply to comment #2) > Sorry that was a bad trace from Win.. Here's a trace on OSX 10.7 dbg: > > crash log for DumpRenderTree (pid 2583): > STDOUT: <empty> > STDERR: objc[2583]: Class MockCrApp is implemented in both /Volumes/data/b/build/slave/WebKit_Mac10_7__dbg_/build/src/xcodebuild/Debug/libwebkit.dylib and /Volumes/data/b/build/slave/WebKit_Mac10_7__dbg_/build/src/xcodebuild/Debug/DumpRenderTree.app/Contents/MacOS/DumpRenderTree. One of the two will be used. Which one is undefined. > STDERR: ASSERTION FAILED: !element || element->isHTMLUnknownElement() > STDERR: ../../third_party/WebKit/Source/WebCore/html/HTMLUnknownElement.h(57) : WebCore::HTMLUnknownElement *WebCore::toHTMLUnknownElement(WebCore::HTMLElement *) > STDERR: 1 0x6f1403f WebCore::toHTMLUnknownElement(WebCore::HTMLElement*) > STDERR: 2 0x6f11449 WebCore::createV8HTMLWrapper(WebCore::HTMLElement*, v8::Handle<v8::Object>, v8::Isolate*) > STDERR: 3 0x7a82c14 WebCore::wrap(WebCore::HTMLElement*, v8::Handle<v8::Object>, v8::Isolate*) I wonder why was this call made to toHTMLUnknownElement? Where the code for createV8HTMLWrapper is located?
Adding abarth@ to see if he knows.
Adding scherkus@chromium.org
Discussed this with Elliott on IRC. Will subclass Element instead of HTMLElement for the WebVTTElement, because sometimes it doesn't use general element names that are declared in HTMLNames.in.
Created attachment 184592 [details] Proposed fix 0.1
<rdar://problem/13081759>
Created attachment 184597 [details] Proposed fix 0.2
*** Bug 107921 has been marked as a duplicate of this bug. ***
Comment on attachment 184597 [details] Proposed fix 0.2 View in context: https://bugs.webkit.org/attachment.cgi?id=184597&action=review This won't work because now you create a <b> element in the xhtml namespace that isn't an HTMLElement. > Source/WebCore/html/track/TextTrackCue.cpp:512 > + clonedNode = HTMLElement::create(toElement(node)->tagQName(), static_cast<Document*>(m_scriptExecutionContext)); It would be much nicer if you just added a method on TextTrackCue called document() that did this and got rid of these casts all over. > Source/WebCore/html/track/WebVTTElement.cpp:37 > + : Element(tagName, document, CreateElement) This isn't okay since WebVTTParser::constructTreeFromToken still creates qnames in the xhtmlNamespace so your Elements are still in the HTML namespace, but now they're not even HTMLElement instances. :( You should put them in the empty namespace, or some webvtt namespace.
*** Bug 108024 has been marked as a duplicate of this bug. ***
Created attachment 185106 [details] Proposed fix 0.3
Comment on attachment 185106 [details] Proposed fix 0.3 View in context: https://bugs.webkit.org/attachment.cgi?id=185106&action=review > Source/WebCore/html/track/WebVTTElement.cpp:86 > - return adoptRef(new WebVTTElement(tagName, document)); > + RefPtr<WebVTTElement> newElement = adoptRef(new WebVTTElement(nodeType, document)); > + return newElement; Oops, this seem unnecessary.
Comment on attachment 185106 [details] Proposed fix 0.3 Attachment 185106 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16125004
Comment on attachment 185106 [details] Proposed fix 0.3 Attachment 185106 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/16165825
Comment on attachment 185106 [details] Proposed fix 0.3 View in context: https://bugs.webkit.org/attachment.cgi?id=185106&action=review > Source/WebCore/html/track/WebVTTElement.cpp:74 > +} You need a return here to prevent a compiler error on some ports. Please also add an ASSERT_NOT_REACHED(). > Source/WebCore/html/track/WebVTTElement.cpp:79 > +: Element(nodeTypeToTagName(nodeType), document, CreateElement) > +, m_isPastNode(0) > +, m_webVTTNodeType(nodeType) Nit: these should be indented. > Source/WebCore/html/track/WebVTTElement.cpp:124 > + htmlElement.get()->setAttribute(langAttributeName(), getAttribute(langAttributeName())); > + htmlElement.get()->setAttribute(HTMLNames::classAttr, getAttribute(HTMLNames::classAttr)); Why langAttributeName() instead of langAttr? Previously we only set 'class' and 'lang' for voice, lang, and class nodes. Was that incorrect?
> > Source/WebCore/html/track/WebVTTElement.cpp:124 > > + htmlElement.get()->setAttribute(langAttributeName(), getAttribute(langAttributeName())); > > + htmlElement.get()->setAttribute(HTMLNames::classAttr, getAttribute(HTMLNames::classAttr)); > > Why langAttributeName() instead of langAttr? These are not standard html names, they are not defined in HTMLAttributeNames.in. Or would you like to rename langAttributeName to langAttr? > Previously we only set 'class' and 'lang' for voice, lang, and class nodes. Was that incorrect? I think we set class for all nodes before. But in case of lang — yes, it should be set to all nodes, that was a bug before.
(In reply to comment #19) > > > Source/WebCore/html/track/WebVTTElement.cpp:124 > > > + htmlElement.get()->setAttribute(langAttributeName(), getAttribute(langAttributeName())); > > > + htmlElement.get()->setAttribute(HTMLNames::classAttr, getAttribute(HTMLNames::classAttr)); > > > > Why langAttributeName() instead of langAttr? > > These are not standard html names, they are not defined in HTMLAttributeNames.in. Or would you like to rename langAttributeName to langAttr? > I am not sure what you mean. This method creates the equivalent HTMLElement. In the current code we have: setAttribute(langAttr, ... so why would we not now have: setAttribute(HTMLNames::langAttr, ...
(In reply to comment #20) > setAttribute(langAttr, ... > > so why would we not now have: > > setAttribute(HTMLNames::langAttr, ... Oh, I didn't release we have 'lang' is the html namespace. I guess langAttributeName() may be removed then.
Comment on attachment 185106 [details] Proposed fix 0.3 Attachment 185106 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16149937
Comment on attachment 185106 [details] Proposed fix 0.3 Attachment 185106 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16188007
Comment on attachment 185106 [details] Proposed fix 0.3 Attachment 185106 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16161966
Comment on attachment 185106 [details] Proposed fix 0.3 Attachment 185106 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16185121
Comment on attachment 185106 [details] Proposed fix 0.3 View in context: https://bugs.webkit.org/attachment.cgi?id=185106&action=review Looking pretty good, just a couple things. > Source/WebCore/html/track/TextTrackCue.cpp:507 > + clonedNode = toWebVTTElement(node)->createEquivalentHTMLElement(document()); Is there a reason to pass in the document() instead of just using the document() inside the node? > Source/WebCore/html/track/TextTrackCue.h:255 > + inline Document* document() { return static_cast<Document*>(m_scriptExecutionContext); } This method is not needed, you already have ownerDocument() right above. Also you don't need "inline" if the method is inside the class def. > Source/WebCore/html/track/WebVTTElement.cpp:64 > + break; Remove the break statements after the returns. You don't need them. >> Source/WebCore/html/track/WebVTTElement.cpp:74 >> +} > > You need a return here to prevent a compiler error on some ports. Please also add an ASSERT_NOT_REACHED(). I don't think you actually need that return, ex. ShadowRoot::childTypeAllowed >> Source/WebCore/html/track/WebVTTElement.cpp:124 >> + htmlElement.get()->setAttribute(HTMLNames::classAttr, getAttribute(HTMLNames::classAttr)); > > Why langAttributeName() instead of langAttr? > > Previously we only set 'class' and 'lang' for voice, lang, and class nodes. Was that incorrect? This is an HTML Element, so you need to use HTMLNames::langAttr. > Source/WebCore/html/track/WebVTTElement.h:33 > + WebVTTNodeTypeNone, Make None = 0 by default. > Source/WebCore/html/track/WebVTTElement.h:77 > + unsigned m_webVTTNodeType:4; Spaces around the colon.
(In reply to comment #26) > > Source/WebCore/html/track/TextTrackCue.cpp:507 > > + clonedNode = toWebVTTElement(node)->createEquivalentHTMLElement(document()); > > Is there a reason to pass in the document() instead of just using the document() inside the node? Document is an m_scriptExecutionContext which is a member of TextTrackCue. I guess it will not be visible inside the element.
Created attachment 185600 [details] Proposed fix 0.4
Comment on attachment 185600 [details] Proposed fix 0.4 View in context: https://bugs.webkit.org/attachment.cgi?id=185600&action=review > Source/WebCore/html/track/WebVTTElement.cpp:93 > + htmlElement = HTMLElement::create(HTMLNames::spanTag, document); Just call document() here, you shouldn't need to pass it in.
Comment on attachment 185600 [details] Proposed fix 0.4 Attachment 185600 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16198933
Comment on attachment 185600 [details] Proposed fix 0.4 Attachment 185600 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/16249020
Created attachment 185635 [details] Proposed fix 0.5
Comment on attachment 185635 [details] Proposed fix 0.5 View in context: https://bugs.webkit.org/attachment.cgi?id=185635&action=review > Source/WebCore/html/track/WebVTTElement.cpp:36 > +static QualifiedName& nodeTypeToTagName(WebVTTNodeType nodeType) Can we use const QualifiedName& instead? > Source/WebCore/html/track/WebVTTParser.cpp:361 > + if (token.name()[0] == 'r' && token.name()[1] == 't') Is it really too slow to do token.name() == "rt" or token.name() == HTMLNames::rtTag.localName()?
(In reply to comment #33) > > Source/WebCore/html/track/WebVTTParser.cpp:361 > > + if (token.name()[0] == 'r' && token.name()[1] == 't') > > Is it really too slow to do token.name() == "rt" or token.name() == HTMLNames::rtTag.localName()? I am not really sure how you can compare pointers like this. The token comes from the parser, we don't use constant values like HTMLNames::rtTag.localName() to populate its name, if we did your second suggestion would work. We could construct AtomicStrings or Just Strings from the token and compare but I am not sure it's worth doing. I will use hash tables later if we end up with lots of different tokens in WebVTT in future.
(In reply to comment #34) > (In reply to comment #33) > > > Source/WebCore/html/track/WebVTTParser.cpp:361 > > > + if (token.name()[0] == 'r' && token.name()[1] == 't') > > > > Is it really too slow to do token.name() == "rt" or token.name() == HTMLNames::rtTag.localName()? > > I am not really sure how you can compare pointers like this. The token comes from the parser, we don't use constant values like HTMLNames::rtTag.localName() to populate its name, if we did your second suggestion would work. We could construct AtomicStrings or Just Strings from the token and compare but I am not sure it's worth doing. I will use hash tables later if we end up with lots of different tokens in WebVTT in future. Ah, ok.
Comment on attachment 185635 [details] Proposed fix 0.5 View in context: https://bugs.webkit.org/attachment.cgi?id=185635&action=review > Source/WebCore/html/track/WebVTTParser.cpp:357 > + switch (token.name().size()) { > + case 1: > + if (token.name()[0] == 'c') > + return WebVTTNodeTypeClass; > + if (token.name()[0] == 'v') > + return WebVTTNodeTypeVoice; > + if (token.name()[0] == 'b') > + return WebVTTNodeTypeBold; > + if (token.name()[0] == 'i') > + return WebVTTNodeTypeItalic; > + if (token.name()[0] == 'u') Nit: this function does a lot of "token.name()". I would be cleaner to put the token name into a local variable.
Created attachment 185875 [details] Proposed fix 0.5
Comment on attachment 185875 [details] Proposed fix 0.5 Clearing flags on attachment: 185875 Committed r141529: <http://trac.webkit.org/changeset/141529>
All reviewed patches have been landed. Closing bug.