Bug 118056

Summary:

REGRESSION: Assertion failure !collection->hasExactlyOneItem() in WebCore::namedItemGetter

Product:

WebKit

Reporter:

Renata Hodovan <rhodovan.u-szeged>

Component:

Bindings

Assignee:

Rob Buis <rwlbuis>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, cdumez, commit-queue, eoconnor, esprehn+autocc, kling, koivisto, rniwa, rwlbuis, sam, webkit-bug-importer

Priority:

Keywords:

Regression, WebExposed

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

116980

Attachments:

Description	Flags
Test case	none
Patch	none
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Renata Hodovan

Reported 2013-06-26 05:59:31 PDT

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

Attachments
Test case (181 bytes, text/html) 2013-06-26 06:00 PDT, Renata Hodovan	no flags	Details
Patch (5.83 KB, patch) 2013-08-12 19:33 PDT, Rob Buis	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (521.85 KB, application/zip) 2013-08-12 21:43 PDT, Build Bot	no flags	Details
Patch (3.60 KB, patch) 2013-08-13 09:41 PDT, Rob Buis	no flags	Details Formatted Diff Diff
Patch (5.35 KB, patch) 2013-08-13 10:38 PDT, Rob Buis	no flags	Details Formatted Diff Diff
Patch (3.99 KB, patch) 2013-08-13 11:24 PDT, Rob Buis	no flags	Details Formatted Diff Diff
Patch (5.78 KB, patch) 2013-08-21 10:02 PDT, Rob Buis	no flags	Details Formatted Diff Diff
Patch (7.64 KB, patch) 2013-08-21 15:35 PDT, Rob Buis	no flags	Details Formatted Diff Diff
Patch (4.40 KB, patch) 2013-08-22 10:30 PDT, Rob Buis	no flags	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Renata Hodovan

Comment 1 2013-06-26 06:00:29 PDT

Created attachment 205481 [details] Test case

Rob Buis

Comment 2 2013-08-12 19:33:11 PDT

Created attachment 208586 [details] Patch

Ryosuke Niwa

Comment 3 2013-08-12 19:37:40 PDT

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.

Ryosuke Niwa

Comment 4 2013-08-12 19:39:34 PDT

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?

Build Bot

Comment 5 2013-08-12 21:43:55 PDT

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

Build Bot

Comment 6 2013-08-12 21:43:57 PDT

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

Ryosuke Niwa

Comment 7 2013-08-12 21:57:51 PDT

Comment on attachment 208586 [details] Patch r- for now.

Rob Buis

Comment 8 2013-08-13 09:05:38 PDT

(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.

Alexey Proskuryakov

Comment 9 2013-08-13 09:33:11 PDT

> 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?

Rob Buis

Comment 10 2013-08-13 09:38:19 PDT

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."

Rob Buis

Comment 11 2013-08-13 09:41:42 PDT

Created attachment 208643 [details] Patch

Darin Adler

Comment 12 2013-08-13 09:43:14 PDT

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.

Chris Dumez

Comment 13 2013-08-13 10:11:51 PDT

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

Rob Buis

Comment 14 2013-08-13 10:20:36 PDT

(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.

Rob Buis

Comment 15 2013-08-13 10:38:26 PDT

Created attachment 208650 [details] Patch

Ryosuke Niwa

Comment 16 2013-08-13 10:56:46 PDT

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.

Rob Buis

Comment 17 2013-08-13 11:08:26 PDT

(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.

Rob Buis

Comment 18 2013-08-13 11:24:22 PDT

Created attachment 208659 [details] Patch

Ryosuke Niwa

Comment 19 2013-08-13 11:48:49 PDT

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.

Rob Buis

Comment 20 2013-08-13 12:51:21 PDT

(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.

Ryosuke Niwa

Comment 21 2013-08-20 19:24:03 PDT

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.

Rob Buis

Comment 22 2013-08-21 09:15:54 PDT

(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.

Ryosuke Niwa

Comment 23 2013-08-21 09:31:23 PDT

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.

Rob Buis

Comment 24 2013-08-21 09:50:02 PDT

(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.

Rob Buis

Comment 25 2013-08-21 10:02:59 PDT

Created attachment 209282 [details] Patch

Ryosuke Niwa

Comment 26 2013-08-21 10:10:52 PDT

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 > +  I don't think these comments are helpful.

Rob Buis

Comment 27 2013-08-21 15:35:04 PDT

Created attachment 209307 [details] Patch

WebKit Commit Bot

Comment 28 2013-08-22 04:49:39 PDT

Comment on attachment 209307 [details] Patch Clearing flags on attachment: 209307 Committed r154441: <http://trac.webkit.org/changeset/154441>

WebKit Commit Bot

Comment 29 2013-08-22 04:49:44 PDT

All reviewed patches have been landed. Closing bug.

Ryosuke Niwa

Comment 30 2013-08-22 06:45:49 PDT

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.

Rob Buis

Comment 31 2013-08-22 10:20:38 PDT

(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.

Ryosuke Niwa

Comment 32 2013-08-22 10:28:42 PDT

(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.

Rob Buis

Comment 33 2013-08-22 10:30:40 PDT

Reopening to attach new patch.

Rob Buis

Comment 34 2013-08-22 10:30:43 PDT

Created attachment 209377 [details] Patch

Rob Buis

Comment 35 2013-08-22 10:33:37 PDT

(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.

Note You need to log in before you can comment on or make changes to this bug.