Bug 83108

Summary: Web Inspector: JSC Crash inspecting node with object event listener
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web Inspector (Deprecated)Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, dglazkov, ggaren, joepeck, keishi, loislo, oliver, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
[REDUCTION] Test Case
none
[PATCH] Proposed Fix
ggaren: review-, joepeck: commit-queue-
[PATCH] Proposed Fix
ggaren: review+, webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-04 none

Description Joseph Pecoraro 2012-04-03 19:53:26 PDT
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
Comment 1 Joseph Pecoraro 2012-04-03 19:53:34 PDT
<rdar://problem/11147318>
Comment 2 Joseph Pecoraro 2012-04-03 19:59:13 PDT
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.
Comment 3 Joseph Pecoraro 2012-04-03 20:00:20 PDT
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.
Comment 4 Geoffrey Garen 2012-04-03 20:11:34 PDT
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.
Comment 5 Geoffrey Garen 2012-04-03 20:16:07 PDT
> 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.
Comment 6 Joseph Pecoraro 2012-04-03 20:22:48 PDT
(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!
Comment 7 Joseph Pecoraro 2012-04-03 20:36:17 PDT
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
Comment 8 Geoffrey Garen 2012-04-03 20:56:14 PDT
Comment on attachment 135501 [details]
[PATCH] Proposed Fix

r=me
Comment 9 WebKit Review Bot 2012-04-04 00:00:03 PDT
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
Comment 10 WebKit Review Bot 2012-04-04 00:00:10 PDT
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
Comment 11 Joseph Pecoraro 2012-04-04 10:42:13 PDT
(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.
Comment 12 Joseph Pecoraro 2012-04-04 11:43:26 PDT
Landed in r113220 <http://trac.webkit.org/changeset/113220>.