Created attachment 95535 [details] [TEST] Test Case STEPS TO REPRODUCE: 1. Inspect the Button on the attached page. 2. Expand Event Listeners in the Elements Panel Sidebar 3. Expand the "document" listener => CRASH CRASH: Exception Type: EXC_BAD_ACCESS (SIGSEGV) Exception Codes: KERN_INVALID_ADDRESS at 0x0000000000000000 Crashed Thread: 0 Dispatch queue: com.apple.main-thread Thread 0 Crashed: Dispatch queue: com.apple.main-thread 0 WebCore::InspectorObject::writeJSON(WTF::Vector<unsigned short, 0ul>*) const + 581 (InspectorValues.cpp:716) 1 WebCore::InspectorObject::writeJSON(WTF::Vector<unsigned short, 0ul>*) const + 597 (InspectorValues.cpp:716) 2 WebCore::InspectorValue::toJSONString() const + 98 (InspectorValues.cpp:555) 3 WebCore::InspectorBackendDispatcher::sendResponse(long, WTF::PassRefPtr<WebCore::InspectorObject>, WTF::PassRefPtr<WebCore::InspectorArray>, WTF::String) + 543 (InspectorBackendDispatcher.cpp:2812) 4 WebCore::InspectorBackendDispatcher::DOM_resolveNode(long, WebCore::InspectorObject*) + 1702 (InspectorBackendDispatcher.cpp:1533) 5 WebCore::InspectorBackendDispatcher::dispatch(WTF::String const&) + 3127 (InspectorBackendDispatcher.cpp:2794) 6 WebCore::InspectorController::dispatchMessageFromFrontend(WTF::String const&) + 81 (InspectorController.cpp:400) 7 WebCore::InspectorFrontendClientLocal::sendMessageToBackend(WTF::String const&) + 33 (InspectorFrontendClientLocal.cpp:167) 8 WebCore::InspectorFrontendHost::sendMessageToBackend(WTF::String const&) + 62 (InspectorFrontendHost.cpp:247) 9 WebCore::jsInspectorFrontendHostPrototypeFunctionSendMessageToBackend(JSC::ExecState*) + 708 (JSInspectorFrontendHost.cpp:478) 10 0 + 62762422112744 11 JSC::JITCode::execute(JSC::RegisterFile*, JSC::ExecState*, JSC::JSGlobalData*) + 108 (JITCode.h:77) 12 JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 1769 (Interpreter.cpp:852) 13 JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 229 (CallData.cpp:38) 14 JSC::JSObject::put(JSC::ExecState*, JSC::Identifier const&, JSC::JSValue, JSC::PutPropertySlot&) + 1783 (JSObject.cpp:150) 15 JSC::JSValue::put(JSC::ExecState*, JSC::Identifier const&, JSC::JSValue, JSC::PutPropertySlot&) + 176 (JSObject.h:812) 16 cti_op_put_by_id + 286 (JITStubs.cpp:1439) 17 jscGeneratedNativeCode + 0 (JITStubs.cpp:952) 18 JSC::JITCode::execute(JSC::RegisterFile*, JSC::ExecState*, JSC::JSGlobalData*) + 108 (JITCode.h:77) 19 JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 1769 (Interpreter.cpp:852) 20 JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 229 (CallData.cpp:38) 21 WebCore::JSMainThreadExecState::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 95 (JSMainThreadExecState.h:48) 22 WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext*, WebCore::Event*) + 2126 (JSEventListener.cpp:127) 23 WebCore::EventTarget::fireEventListeners(WebCore::Event*, WebCore::EventTargetData*, WTF::Vector<WebCore::RegisteredEventListener, 1ul>&) + 508 (EventTarget.cpp:389) 24 WebCore::EventTarget::fireEventListeners(WebCore::Event*) + 520 (EventTarget.cpp:358) 25 WebCore::Node::handleLocalEvents(WebCore::Event*) + 161 (Node.cpp:2707) 26 WebCore::EventDispatcher::dispatchEvent(WTF::PassRefPtr<WebCore::Event>) + 2006 (EventDispatcher.cpp:307) 27 WebCore::MouseEventDispatchMediator::dispatchEvent(WebCore::EventDispatcher*) const + 433 (MouseEvent.cpp:183) 28 WebCore::EventDispatcher::dispatchEvent(WebCore::Node*, WebCore::EventDispatchMediator const&) + 167 (EventDispatcher.cpp:54) 29 WebCore::Node::dispatchMouseEvent(WebCore::PlatformMouseEvent const&, WTF::AtomicString const&, int, WebCore::Node*) + 173 (Node.cpp:2755) 30 WebCore::EventHandler::dispatchMouseEvent(WTF::AtomicString const&, WebCore::Node*, bool, int, WebCore::PlatformMouseEvent const&, bool) + 275 (EventHandler.cpp:2062) 31 WebCore::EventHandler::handleMouseReleaseEvent(WebCore::PlatformMouseEvent const&) + 1509 (EventHandler.cpp:1726) 32 WebCore::EventHandler::mouseUp(NSEvent*) + 367 (EventHandlerMac.mm:526) 33 -[WebHTMLView mouseUp:] + 349 (WebHTMLView.mm:3658) 34 -[NSWindow sendEvent:] + 5547 35 -[NSApplication sendEvent:] + 4719 36 0x100000000 + 233078 37 -[NSApplication run] + 474 38 NSApplicationMain + 364 39 0x100000000 + 40732
InspectorBackendDispatcher::DOM_resolveNode calls InspectorDOMAgent::resolveNode. The DOM Agent finds a "Node*" for document, but when it attempts to resolveNode(node) the document node's ownerDocument() is NULL and it early returns null: PassRefPtr<InspectorObject> InspectorDOMAgent::resolveNode(Node* node) { Document* document = node->ownerDocument(); Frame* frame = document ? document->frame() : 0; if (!frame) return 0; ... } So we then pass out a 0x0, which eventually goes inside an InspectorObject as a value, and causes the crash.
Node::ownerDocument returns NULL if the node is a DOCUMENT_NODE.
Created attachment 95538 [details] [PATCH] Handle Document Nodes in resolveNode() Feel free to send back and force me to implement a test. I can probably shake the dust off my shoulders and figure it out =).
Attachment 95538 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/inspector/InspectorDOMAgent.cpp:1425: Declaration has space between type name and * in Document *document [whitespace/declaration] [3] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 95540 [details] [PATCH] Handle Document Nodes in resolveNode() Whoops, how did I manage to do that? Fixed the style issue.
Comment on attachment 95540 [details] [PATCH] Handle Document Nodes in resolveNode() Adding a test resolving document node would be great.
Created attachment 95642 [details] [PATCH] Now With Test Includes a Test Case for the EventListeners Sidebar in general, because I didn't see one listed. However, the list of event listeners in the sidebar is not in the order of console.log statements that I see when I test this manually. That could probably use some investigation.
Comment on attachment 95642 [details] [PATCH] Now With Test View in context: https://bugs.webkit.org/attachment.cgi?id=95642&action=review Here is a number of hints on the present testing framework. > LayoutTests/http/tests/inspector/elements-test.js:111 > + InspectorTest.expandSelectedElementEventListeners(function() { We generally use named functions with meaningless names in the callbacks :) > LayoutTests/http/tests/inspector/elements-test.js:123 > + InspectorTest.runAfterPendingDispatches(function() { I'd replace this with the sniffer (InspectorTest.addSniffer) that would fire when event listeners data hits front-end. > LayoutTests/http/tests/inspector/elements-test.js:135 > + InspectorTest.runAfterPendingDispatches(function() { I don't think you need to run pending dispatches here. > LayoutTests/http/tests/inspector/elements-test.js:150 > + InspectorTest.runAfterPendingDispatches(callback); And here. > LayoutTests/inspector/elements/event-listener-sidebar-expected.txt:9 > +===== Dump Event Listeners ===== Not sure we need these log entries. > LayoutTests/inspector/elements/event-listener-sidebar-expected.txt:15 > +(anonymous function)documentisAttribute: falselistenerBody: "function (event) { console.log("click - document - capturing"); }"node: HTMLDocumenttype: "click"useCapture: true This formatting could get a bit more love (line breaks). Otherwise "nodeisAttribute" and "truelistenerBody" look confusing. > LayoutTests/inspector/elements/event-listener-sidebar.html:11 > + button.addEventListener("hover", function(event) { console.log("hover - button - bubbling"); }, false); Naming the functions would provide even better test coverage. > LayoutTests/inspector/elements/event-listener-sidebar.html:29 > + InspectorTest.evaluateInPage("setupEventListeners()", callback); There is no need for the front-end to be initialized by the time you run setupEventListeners, so you could simply do that all in the onload(), followed with the runTests(); call.
(In reply to comment #8) > (From update of attachment 95642 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95642&action=review > > Here is a number of hints on the present testing framework. > > > LayoutTests/http/tests/inspector/elements-test.js:111 > > + InspectorTest.expandSelectedElementEventListeners(function() { > > We generally use named functions with meaningless names in the callbacks :) lol! I know I'm kind of breaking the tradition of this totally ugly style, but since this is test code I was trying to stretch what is acceptable. In these cases, like setTimeout(), its much clearer to me whats happening. But, let me know if you do want me to change the style and I will. I should be consistent... > > LayoutTests/http/tests/inspector/elements-test.js:123 > > + InspectorTest.runAfterPendingDispatches(function() { > > I'd replace this with the sniffer (InspectorTest.addSniffer) that would fire when event listeners data hits front-end. That sounds neat, I guess it would be nice, but I don't think it would be that much different. Maybe it would be clearer. > > LayoutTests/http/tests/inspector/elements-test.js:135 > > + InspectorTest.runAfterPendingDispatches(function() { > > I don't think you need to run pending dispatches here. > > > LayoutTests/http/tests/inspector/elements-test.js:150 > > + InspectorTest.runAfterPendingDispatches(callback); > > And here. In my tests, it seemed like every expand() in the EventListeners section required a runAfterPendingDispatches. Expand on the sidebar pane, eventbar section, and eventually the property lists. I can double check that though. > > LayoutTests/inspector/elements/event-listener-sidebar-expected.txt:9 > > +===== Dump Event Listeners ===== > > Not sure we need these log entries. Sounds good. I can limit these. > > LayoutTests/inspector/elements/event-listener-sidebar-expected.txt:15 > > +(anonymous function)documentisAttribute: falselistenerBody: "function (event) { console.log("click - document - capturing"); }"node: HTMLDocumenttype: "click"useCapture: true > > This formatting could get a bit more love (line breaks). Otherwise "nodeisAttribute" and "truelistenerBody" look confusing. Yah, I took the easy way out and used textContent. This has the advantage that if anything changes in the Event Listener's property list it would show up here and not need test tweaks. However, this test is already tied pretty heavily to the internal representation. I can spend some time to pretty this up. > > LayoutTests/inspector/elements/event-listener-sidebar.html:11 > > + button.addEventListener("hover", function(event) { console.log("hover - button - bubbling"); }, false); > > Naming the functions would provide even better test coverage. Will do. I named one, I should name some of the others, and in different ways. > > > LayoutTests/inspector/elements/event-listener-sidebar.html:29 > > + InspectorTest.evaluateInPage("setupEventListeners()", callback); > > There is no need for the front-end to be initialized by the time you run setupEventListeners, so you could simply do that all in the onload(), followed with the runTests(); call. Oh, that sounds good. I was following what I saw in other tests, but I didn't realize they were doing it because it needed to be done after the frontend loaded. Nice. Thanks for the look. I'll try to post an updated patch tomorrow.
Created attachment 96009 [details] [PATCH] Now With a Better Test I improved the test output. But I didn't change the runAfterDispatches to a sniffer. I couldn't remove the dispatches, and it seemed like extra work to switch to sniffer. Is there is a clear advantage, let me know.
Comment on attachment 96009 [details] [PATCH] Now With a Better Test View in context: https://bugs.webkit.org/attachment.cgi?id=96009&action=review The reason I prefer sniffer to the runAfterPendingDispatches is that it tests exactly what I expect. runAfterPendingDispatches is called not after a particular action I am testing, but after its potential implications. Also, it just makes us run more code while in the test (all subsequent updates for styles and such) and hence reduce the test robustness. > LayoutTests/inspector/elements/event-listener-sidebar.html:27 > + function testSetupEventListeners(next) You no longer need neither testSetupEventListeners nor the testSuite sequence (just selectNode followed with expandAndDump) > LayoutTests/inspector/elements/event-listener-sidebar.html:51 > +<body onload="runTest()"> you want onloadHandler call here.
(In reply to comment #11) > (From update of attachment 96009 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96009&action=review > > The reason I prefer sniffer to the runAfterPendingDispatches is that it tests exactly what I expect. runAfterPendingDispatches is called not after a particular action I am testing, but after its potential implications. Also, it just makes us run more code while in the test (all subsequent updates for styles and such) and hence reduce the test robustness. In this case, we expand a sidebar, wait for updates, expand each section, wait for multiple updates, expand each event bar, wait for multiple updates, and expand each property tree and wait for multiple updates. The sniffers at the inner levels, would need to use counters to only dispatch the next step after everything has expanded. It seems overly complicated. Also, if I use sniffers I should probably move the functions out of elements-tests.js, because the functions could no longer be reliably called more than once in a test. This is because the sniffers they add would need to be sticky, which right now can't be easily removed, and if multiple sniffers get added problems could arise. > > LayoutTests/inspector/elements/event-listener-sidebar.html:27 > > + function testSetupEventListeners(next) > > You no longer need neither testSetupEventListeners nor the testSuite sequence (just selectNode followed with expandAndDump) > > > LayoutTests/inspector/elements/event-listener-sidebar.html:51 > > +<body onload="runTest()"> > > you want onloadHandler call here. You're absolutely right on both of these. I have no excuse for this blunder. Sorry!
Created attachment 96055 [details] [PATCH] Now With a Better Test • Fixed the onloadHandler issue. • Kept runAfterDispatches for the issues mentioned above.
Ping.
Comment on attachment 96055 [details] [PATCH] Now With a Better Test Rejecting attachment 96055 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 2 Last 500 characters of output: out ( 0.3%) 55 text diff mismatch ( 4.4%) 327 skipped (26.2%) => Tests that will only be fixed if they crash (WONTFIX) (8114): 1 test timed out ( 0.0%) 109 text diff mismatch ( 1.3%) 7955 skipped (98.0%) Unexpected flakiness: text diff mismatch (1) inspector/styles/parse-utf8-bom.html = TEXT PASS Regressions: Unexpected text diff mismatch : (1) inspector/elements/event-listener-sidebar.html = TEXT Full output: http://queues.webkit.org/results/8843132
Created attachment 97093 [details] Archive of layout-test-results from ec2-cq-01 The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: ec2-cq-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 97135 [details] [DIFF] Pretty Diff of Chromium Results Looks like Chromium has slightly different results in its Event listeners sidebar. - Line numbers instead of (anonymous function) - Line numbers property in sidebar - sourceName property in sidebar I can try to land chromium specific results pulled from the archive. It may be a day or two before I can get this ready to land though. Quite busy. Thanks for the review!
Comment on attachment 95540 [details] [PATCH] Handle Document Nodes in resolveNode() Cleared Pavel Feldman's review+ from obsolete attachment 95540 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Attachment 96055 [details] was posted by a committer and has review+, assigning to Joseph Pecoraro for commit.
Landed with Chromium expected results for their extra output in r89317: http://trac.webkit.org/changeset/89317