Bug 105353 – Minor improvements to HTMLMediaElement

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Joseph Pecoraro]

While reading through HTMLMediaElement I noticed some cases where there could be string improvements:
<http://trac.webkit.org/wiki/EfficientStrings>

Patch to follow.

[Joseph Pecoraro]

There was also a case (HTMLMediaElement::setLoop) which was doing extra work.

[Joseph Pecoraro]

Created attachment 180039 [details]
[PATCH] Proposed Improvements

[Joseph Pecoraro]

This would be the path from setBooleanAttribute -> parseAttribute, which handles loopAttr:

* thread #1: tid = 0x1c03, 0x000000011216bc42 WebCore`WebCore::HTMLMediaElement::parseAttribute(this=0x00007fa62523e500, name=0x00000001143cf820, value=0x0000000110fcb358) + 34 at HTMLMediaElement.cpp:372, stop reason = breakpoint 2.1
    frame #0: 0x000000011216bc42 WebCore`WebCore::HTMLMediaElement::parseAttribute(this=0x00007fa62523e500, name=0x00000001143cf820, value=0x0000000110fcb358) + 34 at HTMLMediaElement.cpp:372
    frame #1: 0x00000001121f06fb WebCore`WebCore::HTMLVideoElement::parseAttribute(this=0x00007fa62523e500, name=0x00000001143cf820, value=0x0000000110fcb358) + 379 at HTMLVideoElement.cpp:124
    frame #2: 0x0000000111edb1e2 WebCore`WebCore::Element::attributeChanged(this=0x00007fa62523e500, name=0x00000001143cf820, newValue=0x0000000110fcb358) + 130 at Element.cpp:776
    frame #3: 0x0000000112fea845 WebCore`WebCore::StyledElement::attributeChanged(this=0x00007fa62523e500, name=0x00000001143cf820, newValue=0x0000000110fcb358) + 181 at StyledElement.cpp:168
    frame #4: 0x0000000111edfad9 WebCore`WebCore::Element::didAddAttribute(this=0x00007fa62523e500, name=0x00000001143cf820, value=0x0000000110fcb358) + 73 at Element.cpp:2497
    frame #5: 0x0000000111edfa86 WebCore`WebCore::Element::addAttributeInternal(this=0x00007fa62523e500, name=0x00000001143cf820, value=0x0000000110fcb358, inSynchronizationOfLazyAttribute=NotInSynchronizationOfLazyAttribute) + 150 at Element.cpp:1706
    frame #6: 0x0000000111ee481f WebCore`WebCore::Element::setAttributeInternal(this=0x00007fa62523e500, index=18446744073709551615, name=0x00000001143cf820, newValue=0x0000000110fcb358, inSynchronizationOfLazyAttribute=NotInSynchronizationOfLazyAttribute) + 127 at Element.cpp:731
    frame #7: 0x0000000111ed961a WebCore`WebCore::Element::setAttribute(this=0x00007fa62523e500, name=0x00000001143cf820, value=0x0000000110fcb358) + 74 at Element.cpp:714
    frame #8: 0x0000000111ed95ad WebCore`WebCore::Element::setBooleanAttribute(this=0x00007fa62523e500, name=0x00000001143cf820, value=true) + 61 at Element.cpp:288

[Eric Carlson]

Comment on attachment 180039 [details]
[PATCH] Proposed Improvements

Nice, thank you!

[WebKit Review Bot]

Comment on attachment 180039 [details]
[PATCH] Proposed Improvements

Clearing flags on attachment: 180039

Committed r138097: <http://trac.webkit.org/changeset/138097>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Filip Pizlo]

I think you guys just set my machine on fire:


[pizlo@bigmac OpenSource] DYLD_FRAMEWORK_PATH=WebKitBuild/Debug/ WebKitBuild/Debug/DumpRenderTree LayoutTests/media/media-can-play-webm.html 
ASSERTION FAILED: m_length
/Volumes/Data/pizlo/quartary/OpenSource/Source/WTF/wtf/text/StringImpl.h(252) : WTF::StringImpl::StringImpl(const char *, unsigned int, WTF::StringImpl::ConstructFromLiteralTag)
1   0x10ebcba44 WTF::StringImpl::StringImpl(char const*, unsigned int, WTF::StringImpl::ConstructFromLiteralTag)
2   0x10ebc9be9 WTF::StringImpl::StringImpl(char const*, unsigned int, WTF::StringImpl::ConstructFromLiteralTag)
3   0x10ebc1a16 WTF::StringImpl::createFromLiteral(char const*)
4   0x10ebd4840 WTF::String::String(WTF::ASCIILiteral)
5   0x10ebd47fd WTF::String::String(WTF::ASCIILiteral)
6   0x11060dd86 WebCore::HTMLMediaElement::canPlayType(WTF::String const&, WTF::String const&, WebCore::KURL const&) const
7   0x110a98029 WebCore::jsHTMLMediaElementPrototypeFunctionCanPlayType(JSC::ExecState*)
8   0x460869801265
9   0x10e8e8184 JSC::JITCode::execute(JSC::JSStack*, JSC::ExecState*, JSC::JSGlobalData*)
10  0x10e8e08a9 JSC::Interpreter::execute(JSC::EvalExecutable*, JSC::ExecState*, JSC::JSValue, JSC::JSScope*)
11  0x10e8dfff7 JSC::eval(JSC::ExecState*)
12  0x10eae7d75 llint_slow_path_call_eval
13  0x10eaf00a7 llint_op_call_eval
14  0x10e8e8184 JSC::JITCode::execute(JSC::JSStack*, JSC::ExecState*, JSC::JSGlobalData*)
15  0x10e8e53ff JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
16  0x10e763a12 JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
17  0x11089ad92 WebCore::JSMainThreadExecState::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
18  0x1109dc796 WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext*, WebCore::Event*)
19  0x1103cf363 WebCore::EventTarget::fireEventListeners(WebCore::Event*, WebCore::EventTargetData*, WTF::Vector<WebCore::RegisteredEventListener, 1ul>&)
20  0x1103cf135 WebCore::EventTarget::fireEventListeners(WebCore::Event*)
21  0x110319bd0 WebCore::DOMWindow::dispatchEvent(WTF::PassRefPtr<WebCore::Event>, WTF::PassRefPtr<WebCore::EventTarget>)
22  0x110320be8 WebCore::DOMWindow::dispatchLoadEvent()
23  0x11017a0bf WebCore::Document::dispatchWindowLoadEvent()
24  0x110177bf0 WebCore::Document::implicitClose()
25  0x11049572b WebCore::FrameLoader::checkCallImplicitClose()
26  0x1104953f3 WebCore::FrameLoader::checkCompleted()
27  0x110495595 WebCore::FrameLoader::loadDone()
28  0x10fec68e2 WebCore::CachedResourceLoader::loadDone(WebCore::CachedResource*)
29  0x1114eb28f WebCore::SubresourceLoader::releaseResources()
30  0x1112be579 WebCore::ResourceLoader::didFinishLoading(double)
31  0x1114eae75 WebCore::SubresourceLoader::didFinishLoading(double)

[WebKit Review Bot]

Re-opened since this is blocked by bug 105386

[Joseph Pecoraro]

Ahh, from the backtrace above it sounds like this was ASCIILiteral("") in HTMLMediaElement::canPlayType. StringImpl ends up doing strlen, gets a length of 0, and uses a constructor that asserts a non-0 length.

[Oliver Hunt]

Fixed the zero length ASCIILiteral bug in  r138187

You should be able to roll back in now.

[Joseph Pecoraro]

Rolling back in after Oliver's change.
Landed r139006: <http://trac.webkit.org/changeset/139006>