Bug 83108 – Web Inspector: JSC Crash inspecting node with object event listener

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

[Joseph Pecoraro]

Created attachment 135490 [details]
[REDUCTION] Test Case

CRASH inspecting body of this page (attachment):

    <p>Inspect Body and you will Crash</p>
    <script>
    function Foo() { document.body.addEventListener("click", this, true); }
    new Foo();
    </script>

Backtrace:

    Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
    0   com.apple.WebCore WebCore::eventListenerHandlerLocation(WebCore::Document*, WebCore::EventListener*, WTF::String&, int&) + 128
    1   com.apple.WebCore WebCore::InspectorDOMAgent::buildObjectForEventListener(WebCore::RegisteredEventListener const&, WTF::AtomicString const&, WebCore::Node*) + 373
    2   com.apple.WebCore WebCore::InspectorDOMAgent::getEventListenersForNode(WTF::String*, int, WTF::RefPtr<WebCore::TypeBuilder::Array<WebCore::TypeBuilder::DOM::EventListener> >&) + 286
    3   com.apple.WebCore WebCore::InspectorBackendDispatcherImpl::DOM_getEventListenersForNode(long, WebCore::InspectorObject*) + 393
    4   com.apple.WebCore WebCore::InspectorBackendDispatcherImpl::dispatch(WTF::String const&) + 2131
    5   com.apple.WebCore WebCore::InspectorBackendDispatchTask::onTimer(WebCore::Timer<WebCore::InspectorBackendDispatchTask>*) + 73
    6   com.apple.WebCore WebCore::ThreadTimers::sharedTimerFiredInternal() + 148
    7   com.apple.WebCore _ZN7WebCoreL10timerFiredEP16__CFRunLoopTimerPv + 51

[Joseph Pecoraro]

<rdar://problem/11147318>

[Joseph Pecoraro]

Created attachment 135493 [details]
[PATCH] Proposed Fix

I should probably add an isFunction() check to JSObject.h instead of adding this code in WebCore/bindings/js so cq-. But I'll see how the bots do with this.

[Joseph Pecoraro]

Comment on attachment 135493 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=135493&action=review

> LayoutTests/inspector/elements/event-listener-sidebar.html:27
> +    function ObjectHandler() { document.addEventListener("click", this, true); }
> +    ObjectHandler.prototype.toString = function() { return "ObjectHandler"; }
> +    new ObjectHandler();

Oops, I forgot to git add the new results! I'm making a new patch now.

[Geoffrey Garen]

Comment on attachment 135493 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=135493&action=review

> Source/WebCore/bindings/js/ScriptEventListener.cpp:-103
> -    JSLock lock(SilenceAssertionsOnly);

Why did you remove this lock?

> Source/WebCore/bindings/js/ScriptEventListener.cpp:125
> +    if (!isJSFunction(jsObject))

No need to check isJSFunction. jsCast<T> will return 0 if the cast fails.

[Geoffrey Garen]

> No need to check isJSFunction. jsCast<T> will return 0 if the cast fails.

Sorry, I meant to say "jsDynamicCast". That's the best thing to use here.

[Joseph Pecoraro]

(In reply to comment #5)
> > No need to check isJSFunction. jsCast<T> will return 0 if the cast fails.
> 
> Sorry, I meant to say "jsDynamicCast". That's the best thing to use here.

Oh awesome!

[Joseph Pecoraro]

Created attachment 135501 [details]
[PATCH] Proposed Fix

- Keep JSLock and add one for the same reason (possible allocations under jsFunction) to eventListenerHandlerLocation.
- Switch to jsDynamicCast.
- Include updated LayoutTest results

[Geoffrey Garen]

Comment on attachment 135501 [details]
[PATCH] Proposed Fix

r=me

[WebKit Review Bot]

Comment on attachment 135501 [details]
[PATCH] Proposed Fix

Attachment 135501 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12320670

New failing tests:
inspector/elements/event-listener-sidebar.html

[WebKit Review Bot]

Created attachment 135524 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Joseph Pecoraro]

(In reply to comment #10)
> The attached test failures were seen while running run-webkit-tests on the chromium-ews.

I had to update the chromium expected results at:

    LayoutTests/platform/chromium/inspector/elements/event-listener-sidebar-expected.txt

I took the expected results from the EWS bots, and they look correct.

[Joseph Pecoraro]

Landed in r113220 <http://trac.webkit.org/changeset/113220>.