Bug 61834 – Web Inspector: CRASH if Expanding Event Listener on document

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Joseph Pecoraro]

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

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Node::ownerDocument returns NULL if the node is a DOCUMENT_NODE.

[Joseph Pecoraro]

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 =).

[WebKit Review Bot]

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.

[Joseph Pecoraro]

Created attachment 95540 [details]
[PATCH] Handle Document Nodes in resolveNode()

Whoops, how did I manage to do that? Fixed the style issue.

[Pavel Feldman]

Comment on attachment 95540 [details]
[PATCH] Handle Document Nodes in resolveNode()

Adding a test resolving document node would be great.

[Joseph Pecoraro]

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.

[Pavel Feldman]

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.

[Joseph Pecoraro]

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

[Joseph Pecoraro]

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.

[Pavel Feldman]

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.

[Joseph Pecoraro]

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

[Joseph Pecoraro]

Created attachment 96055 [details]
[PATCH] Now With a Better Test

• Fixed the onloadHandler issue.
• Kept runAfterDispatches for the issues mentioned above.

[Joseph Pecoraro]

Ping.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Joseph Pecoraro]

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!

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Attachment 96055 [details] was posted by a committer and has review+, assigning to Joseph Pecoraro for commit.

[Joseph Pecoraro]

Landed with Chromium expected results for their extra output in r89317:
http://trac.webkit.org/changeset/89317