The test below fails on the assertion above: <html> <a id="logger"></a> <svg id="logger"></svg> <body onselectionchange="logger(foo)"></body> <select contenteditable="plaintext-only" autofocus="autofocus"></select> </html> BackTrace: Program received signal SIGSEGV, Segmentation fault. 0x00007ffff5760ba5 in WTFCrash () at /home/reni/Data/REPOS/webkit_sec/Source/WTF/wtf/Assertions.cpp:339 339 *(int *)(uintptr_t)0xbbadbeef = 0; (gdb) bt #0 0x00007ffff5760ba5 in WTFCrash () at /home/reni/Data/REPOS/webkit_sec/Source/WTF/wtf/Assertions.cpp:339 #1 0x00007ffff3fe48d6 in WebCore::namedItemGetter (exec=0x7fffe4180060, slotBase=..., propertyName=...) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:105 #2 0x00007ffff3e3e158 in JSC::PropertySlot::getValue (this=0x7fffffffc520, exec=0x7fffe4180060, propertyName=...) at /home/reni/Data/REPOS/webkit_sec/Source/JavaScriptCore/runtime/PropertySlot.h:76 #3 0x00007ffff56f48fb in JSC::JSScope::resolveWithThis (callFrame=0x7fffe4180060, identifier=..., base=0x7fffe4180070, operations=0x7668c0) at /home/reni/Data/REPOS/webkit_sec/Source/JavaScriptCore/runtime/JSScope.cpp:531 #4 0x00007ffff5607c86 in JSC::LLInt::llint_slow_path_resolve_with_this (exec=0x7fffe4180060, pc=0x772d80) at /home/reni/Data/REPOS/webkit_sec/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:876 #5 0x00007ffff5612eea in llint_op_resolve_with_this () from /home/reni/Data/REPOS/webkit_sec/WebKitBuild/Debug/lib/libQt5WebKit.so.5 #6 0x00007fffe4180060 in ?? () #7 0x000000000074b960 in ?? () #8 0x00007fffffffc6f0 in ?? () #9 0x00007ffff55bcd6d in JSC::JSStack::installTrapsAfterFrame (this=0x0, frame=0x0) at /home/reni/Data/REPOS/webkit_sec/Source/JavaScriptCore/interpreter/JSStackInlines.h:212 #10 0x00007ffff55bbccc in JSC::JITCode::execute (this=0x7fff863efe90, stack=0x74b960, callFrame=0x7fffe4180060, vm=0x7d3110) at /home/reni/Data/REPOS/webkit_sec/Source/JavaScriptCore/jit/JITCode.h:135 #11 0x00007ffff55b9999 in JSC::Interpreter::executeCall (this=0x74b950, callFrame=0x7fffe405f8d8, function=0x7fff9c06f2b0, callType=JSC::CallTypeJS, callData=..., thisValue=..., args=...) at /home/reni/Data/REPOS/webkit_sec/Source/JavaScriptCore/interpreter/Interpreter.cpp:1052 #12 0x00007ffff568f867 in JSC::call (exec=0x7fffe405f8d8, functionObject=..., callType=JSC::CallTypeJS, callData=..., thisValue=..., args=...) at /home/reni/Data/REPOS/webkit_sec/Source/JavaScriptCore/runtime/CallData.cpp:40 #13 0x00007ffff3fcf08b in WebCore::JSMainThreadExecState::call (exec=0x7fffe405f8d8, functionObject=..., callType=JSC::CallTypeJS, callData=..., thisValue=..., args=...) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/bindings/js/JSMainThreadExecState.h:56 #14 0x00007ffff3ffda81 in WebCore::JSEventListener::handleEvent (this=0x8a4100, scriptExecutionContext=0x8631d0, event=0x8bcd90) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/bindings/js/JSEventListener.cpp:130 #15 0x00007ffff42b6400 in WebCore::EventTarget::fireEventListeners (this=0x863120, event=0x8bcd90, d=0x8a41a0, entry=...) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/dom/EventTarget.cpp:248 #16 0x00007ffff42b606d in WebCore::EventTarget::fireEventListeners (this=0x863120, event=0x8bcd90) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/dom/EventTarget.cpp:190 #17 0x00007ffff42e16cd in WebCore::Node::handleLocalEvents (this=0x863120, event=0x8bcd90) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/dom/Node.cpp:2205 #18 0x00007ffff42a8cd6 in WebCore::EventContext::handleLocalEvents (this=0x756d80, event=0x8bcd90) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/dom/EventContext.cpp:58 #19 0x00007ffff42aaa97 in WebCore::EventDispatcher::dispatchEventAtTarget (this=0x7fffffffcd90) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/dom/EventDispatcher.cpp:162 #20 0x00007ffff42aa754 in WebCore::EventDispatcher::dispatch (this=0x7fffffffcd90) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/dom/EventDispatcher.cpp:119 #21 0x00007ffff42a9651 in WebCore::EventDispatchMediator::dispatchEvent (this=0x711e80, dispatcher=0x7fffffffcd90) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/dom/EventDispatchMediator.cpp:54 #22 0x00007ffff42a9d01 in WebCore::EventDispatcher::dispatchEvent (node=0x863120, mediator=...) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/dom/EventDispatcher.cpp:53 #23 0x00007ffff42e18e2 in WebCore::Node::dispatchEvent (this=0x863120, event=...) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/dom/Node.cpp:2226 #24 0x00007ffff4274e29 in WebCore::DocumentEventQueue::dispatchEvent (this=0x7890a0, event=...) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/dom/DocumentEventQueue.cpp:147 #25 0x00007ffff4274cf2 in WebCore::DocumentEventQueue::pendingEventTimerFired (this=0x7890a0) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/dom/DocumentEventQueue.cpp:137 #26 0x00007ffff4274506 in WebCore::DocumentEventQueueTimer::fired (this=0x789130) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/dom/DocumentEventQueue.cpp:48 #27 0x00007ffff4838a04 in WebCore::ThreadTimers::sharedTimerFiredInternal (this=0x6d6990) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/platform/ThreadTimers.cpp:129 #28 0x00007ffff48388f1 in WebCore::ThreadTimers::sharedTimerFired () at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/platform/ThreadTimers.cpp:105 #29 0x00007ffff4b2ab22 in WebCore::SharedTimerQt::timerEvent (this=0x6d69c0, ev=0x7fffffffd760) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/platform/qt/SharedTimerQt.cpp:113 #30 0x00007ffff229b66c in QObject::event(QEvent*) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5 #31 0x00007ffff30e1dbc in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Widgets.so.5 #32 0x00007ffff30e5075 in QApplication::notify(QObject*, QEvent*) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Widgets.so.5 ---Type <return> to continue, or q <return> to quit--- #33 0x00007ffff2275dbe in QCoreApplication::notifyInternal(QObject*, QEvent*) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5 #34 0x00007ffff22bc75c in QTimerInfoList::activateTimers() () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5 #35 0x00007ffff22bd094 in ?? () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5 #36 0x00007fffee40bf05 in g_main_dispatch (context=0x6632f0) at /build/buildd/glib2.0-2.36.0/./glib/gmain.c:3054 #37 g_main_context_dispatch (context=context@entry=0x6632f0) at /build/buildd/glib2.0-2.36.0/./glib/gmain.c:3630 #38 0x00007fffee40c248 in g_main_context_iterate (context=context@entry=0x6632f0, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at /build/buildd/glib2.0-2.36.0/./glib/gmain.c:3701 #39 0x00007fffee40c304 in g_main_context_iteration (context=0x6632f0, may_block=1) at /build/buildd/glib2.0-2.36.0/./glib/gmain.c:3762 #40 0x00007ffff22bd4bc in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5 #41 0x00007ffff2274d3b in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5 #42 0x00007ffff2278120 in QCoreApplication::exec() () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5 #43 0x0000000000421ba0 in launcherMain (app=...) at /home/reni/Data/REPOS/webkit_sec/Tools/QtTestBrowser/qttestbrowser.cpp:49 #44 0x0000000000423680 in main (argc=2, argv=0x7fffffffdca8) at /home/reni/Data/REPOS/webkit_sec/Tools/QtTestBrowser/qttestbrowser.cpp:318
Created attachment 205481 [details] Test case
Created attachment 208586 [details] Patch
Comment on attachment 208586 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208586&action=review > Source/WebCore/ChangeLog:14 > + This behaviour matches Opera 12 but not FireFox, which included SVGs. > + I'm afraid this isn't compatible with the Web. > Source/WebCore/html/HTMLNameCollection.cpp:57 > +bool WindowNameCollection::nodeMatchesIfIdAttributeMatch(Element* element) > +{ > + return element->isHTMLElement(); > +} This is going to be perf. regression. We need to keep this inlined.
I'm somewhat confused as to why this patch appears to change the behavior of WebKit. Is this behavior a regression; i.e. does restricting name getters to HTML element restores an old behavior of WebKit?
Comment on attachment 208586 [details] Patch Attachment 208586 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1446512 New failing tests: svg/custom/use-listener-append-crash.html svg/animations/deferred-insertion.html svg/animations/svginteger-animation-1.html svg/animations/svginteger-animation-2.html
Created attachment 208591 [details] Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.4
Comment on attachment 208586 [details] Patch r- for now.
(In reply to comment #4) > I'm somewhat confused as to why this patch appears to change the behavior of WebKit. I thought that we should follow DOM4, which mentions only taking id's from HTML elements into account: http://www.w3.org/html/wg/drafts/html/master/browsers.html#named-access-on-the-window-object Then again FireFox has not adopted it yet either. > Is this behavior a regression; i.e. does restricting name getters to HTML element restores an old behavior of WebKit? In a way, yes :) In the sense that before HTML5 it was not possible to embed SVG in html like that. Note that the problem only occurs when there are multiple matches and at least one non html element in the matches. Note that even as it is in trunk right now html elements are preferred ; if there are two html matches a HTMLCollection of 2 is returned, if there are two non html matches a HTMLCollection of 0 will be returned (or ASSERT in Debug). So to me that behaviour seems a bit random/unfair, I'll add a patch next that should be less intrusive.
> I thought that we should follow DOM4 As a general guideline, we should strive to match DOM4, however not necessarily through blindly following it. Whe the draft is wrong, it should be fixed. This particular requirement seems kind of arbitrary - what is an «HTML element» when SVG ones look exactly the same in markup?
Hi Alexey, (In reply to comment #9) > > I thought that we should follow DOM4 > > As a general guideline, we should strive to match DOM4, however not necessarily through blindly following it. Whe the draft is wrong, it should be fixed. This particular requirement seems kind of arbitrary - what is an «HTML element» when SVG ones look exactly the same in markup? I agree in general with the first part, it may just be harder for me since I don't follow the web specification mailing lists as such. I was going by the definition of HTML element from DOM4 to exclude SVG: http://www.w3.org/html/wg/drafts/html/master/infrastructure.html#html-elements "The term "HTML elements", when used in this specification, refers to any element in that namespace, and thus refers to both HTML and XHTML elements."
Created attachment 208643 [details] Patch
Comment on attachment 208643 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208643&action=review > Source/WebCore/html/HTMLCollection.cpp:634 > + if (!nameAttrVal.isEmpty() && idAttrVal != nameAttrVal && (type() != DocAll || nameShouldBeVisibleInDocumentAll(toHTMLElement(element)))) What makes the toHTMLElement cast here safe? I don’t see an isHTMLElement check in this function any more. I think this is an unsafe cast.
Comment on attachment 208643 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208643&action=review > LayoutTests/ChangeLog:10 > + * svg/custom/window-named-item-lookup-expected.txt: Added. outdated changelog
(In reply to comment #13) > (From update of attachment 208643 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208643&action=review > > > LayoutTests/ChangeLog:10 > > + * svg/custom/window-named-item-lookup-expected.txt: Added. > > outdated changelog Thanks, will fix.
Created attachment 208650 [details] Patch
Comment on attachment 208650 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208650&action=review > Source/WebCore/ChangeLog:8 > + Change WindowNameCollection to include both SVG and HTML elements with id's so it matches HTMLDocument::m_windowNamedItem. Please explain why the assertion is hit and how this change fixes it. > Source/WebCore/html/HTMLCollection.cpp:634 > + if (!nameAttrVal.isEmpty() && idAttrVal != nameAttrVal && (type() != DocAll || (!element->isHTMLElement() || nameShouldBeVisibleInDocumentAll(toHTMLElement(element))))) This doesn't make much sense. The whole point nameShouldBeVisibleInDocumentAll is to limit the kind of elements that could be looked up by names.
(In reply to comment #16) > (From update of attachment 208650 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208650&action=review > > > Source/WebCore/ChangeLog:8 > > + Change WindowNameCollection to include both SVG and HTML elements with id's so it matches HTMLDocument::m_windowNamedItem. > > Please explain why the assertion is hit and how this change fixes it. Ok, will try. > > Source/WebCore/html/HTMLCollection.cpp:634 > > + if (!nameAttrVal.isEmpty() && idAttrVal != nameAttrVal && (type() != DocAll || (!element->isHTMLElement() || nameShouldBeVisibleInDocumentAll(toHTMLElement(element))))) > > This doesn't make much sense. The whole point nameShouldBeVisibleInDocumentAll is to limit the kind of elements that could be looked up by names. Oops, you are right, will fix.
Created attachment 208659 [details] Patch
Comment on attachment 208659 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208659&action=review > Source/WebCore/ChangeLog:12 > + To fix this change WindowNameCollection to include both SVG and HTML elements so it matches DocumentOrderedMap. What do Firefox and IE do? We should try to match their behavior if possible.
(In reply to comment #19) > (From update of attachment 208659 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208659&action=review > > > Source/WebCore/ChangeLog:12 > > + To fix this change WindowNameCollection to include both SVG and HTML elements so it matches DocumentOrderedMap. > > What do Firefox and IE do? We should try to match their behavior if possible. FireFox never seems to return a HTMLCollection. In the case of multiple matches it will return the first id in the list. I do not have access to IE, maybe somebody else can check that one.
Comment on attachment 208659 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208659&action=review > Source/WebCore/ChangeLog:9 > + includes matched SVG elements, but the WindowNameCollection used to collect the elements does not. I just confirmed that Firefox finds the SVGTextElement in the following example: <svg><text id="hi">hi</text></svg> <script>alert(window['hi']);</script> So we should fix WindowNameCollection instead.
(In reply to comment #21) > (From update of attachment 208659 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208659&action=review > > > Source/WebCore/ChangeLog:9 > > + includes matched SVG elements, but the WindowNameCollection used to collect the elements does not. > > I just confirmed that Firefox finds the SVGTextElement in the following example: > <svg><text id="hi">hi</text></svg> > <script>alert(window['hi']);</script> > So we should fix WindowNameCollection instead. The example works with the current patch put up for review since that one changes WindowNameCollection to include both SVG and HTML elements. Let me know if anything more needs to be done like adding more tests.
Comment on attachment 208659 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208659&action=review > Source/WebCore/html/HTMLCollection.cpp:634 > + if (!nameAttrVal.isEmpty() && idAttrVal != nameAttrVal && (type() != DocAll || (element->isHTMLElement() && nameShouldBeVisibleInDocumentAll(toHTMLElement(element))))) I think we need to check element->isHTMLElement() for DocAll as well.
(In reply to comment #23) > (From update of attachment 208659 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208659&action=review > > > Source/WebCore/html/HTMLCollection.cpp:634 > > + if (!nameAttrVal.isEmpty() && idAttrVal != nameAttrVal && (type() != DocAll || (element->isHTMLElement() && nameShouldBeVisibleInDocumentAll(toHTMLElement(element))))) > > I think we need to check element->isHTMLElement() for DocAll as well. Agreed, will fix.
Created attachment 209282 [details] Patch
Comment on attachment 209282 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209282&action=review > Source/WebCore/html/HTMLCollection.cpp:636 > + if (!nameAttrVal.isEmpty() && idAttrVal != nameAttrVal && (type() != DocAll || nameShouldBeVisibleInDocumentAll(toHTMLElement(element)))) Can we add a test case for document.all as well? > LayoutTests/svg/custom/window-named-item-lookup.html:1 > +<html> Missing DOCTYPE > LayoutTests/svg/custom/window-named-item-lookup.html:22 > + if (window.test1.length != 2) > + return; > + > + if (window.test2.length != 2) > + return; > + > + if (window.test3.nodeName != "A") > + return; > + > + if (window.test4.length != 2) > + return; > + > + if (window.test5.length != 2) > + return; Can we use shouldBe for each one of these cases? > LayoutTests/svg/custom/window-named-item-lookup.html:30 > + <!-- test 1, one html, followed by one non-html --> I don't think these comments are helpful.
Created attachment 209307 [details] Patch
Comment on attachment 209307 [details] Patch Clearing flags on attachment: 209307 Committed r154441: <http://trac.webkit.org/changeset/154441>
All reviewed patches have been landed. Closing bug.
Comment on attachment 209307 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209307&action=review > Source/WebCore/ChangeLog:14 > + Tests: svg/custom/document-all-includes-svg.html Oops, this test is misplaced in svg/custom! It should be in fast/dom or fast/html instead.
(In reply to comment #30) > (From update of attachment 209307 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=209307&action=review > > > Source/WebCore/ChangeLog:14 > > + Tests: svg/custom/document-all-includes-svg.html > > Oops, this test is misplaced in svg/custom! It should be in fast/dom or fast/html instead. I was afk this morning, will create a follow-up patch soon though.
(In reply to comment #31) > (In reply to comment #30) > > (From update of attachment 209307 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=209307&action=review > > > > > Source/WebCore/ChangeLog:14 > > > + Tests: svg/custom/document-all-includes-svg.html > > > > Oops, this test is misplaced in svg/custom! It should be in fast/dom or fast/html instead. > > I was afk this morning, will create a follow-up patch soon though. Actually, never mind about this comment. I didn't realize it was using svg element. It's probably fine being there since it requires ENABLE_SVG.
Reopening to attach new patch.
Created attachment 209377 [details] Patch
(In reply to comment #32) > (In reply to comment #31) > > (In reply to comment #30) > > > (From update of attachment 209307 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=209307&action=review > > > > > > > Source/WebCore/ChangeLog:14 > > > > + Tests: svg/custom/document-all-includes-svg.html > > > > > > Oops, this test is misplaced in svg/custom! It should be in fast/dom or fast/html instead. > > > > I was afk this morning, will create a follow-up patch soon though. > > Actually, never mind about this comment. I didn't realize it was using svg element. It's probably fine being there since it requires ENABLE_SVG. I don't mind either way, but yes it does require ENABLE_SVG for the test to pass. Given above, can you r- or clear review flag and close the bug? I only saw above comment after the patch upload.