Web Inspector: ASSERT_NOT_REACHED in PageDebuggerAgent::didAddEventListener when...

https://bugs.webkit.org/show_bug.cgi?id=181580
<rdar://problem/36461309>

Reviewed by Brian Burg.

Source/WebCore:

EventTarget should pass newly added EventListeners to InspectorInstrumentation,
instead of PageDebuggerAgent assuming the last item in the EventListenerVector
is the most recently added listener. This assumption does not hold when
the new listener replaces an existing listener.

* dom/EventTarget.cpp:
(WebCore::EventTarget::addEventListener):
(WebCore::EventTarget::setAttributeEventListener):

* inspector/InspectorInstrumentation.cpp:
(WebCore::InspectorInstrumentation::didAddEventListenerImpl):

* inspector/InspectorInstrumentation.h:
(WebCore::InspectorInstrumentation::didAddEventListener):

* inspector/agents/page/PageDebuggerAgent.cpp:
(WebCore::PageDebuggerAgent::didAddEventListener):
* inspector/agents/page/PageDebuggerAgent.h:

LayoutTests:

Add new test covering the case where adding an attribute event listener
causes an existing attribute event listener to be replaced.

* inspector/debugger/async-stack-trace-expected.txt:
* inspector/debugger/async-stack-trace.html:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@231659 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index dd871f0..81a3e50 100644 (file)
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,17 @@
+2018-05-10  Matt Baker  <mattbaker@apple.com>
+
+        Web Inspector: ASSERT_NOT_REACHED in PageDebuggerAgent::didAddEventListener when page adds attribute event listener
+        https://bugs.webkit.org/show_bug.cgi?id=181580
+        <rdar://problem/36461309>
+
+        Reviewed by Brian Burg.
+
+        Add new test covering the case where adding an attribute event listener
+        causes an existing attribute event listener to be replaced.
+
+        * inspector/debugger/async-stack-trace-expected.txt:
+        * inspector/debugger/async-stack-trace.html:
+
 2018-05-10  Chris Dumez  <cdumez@apple.com>
 
         'Cross-Origin-Options header implementation follow-up

diff --git a/LayoutTests/inspector/debugger/async-stack-trace-expected.txt b/LayoutTests/inspector/debugger/async-stack-trace-expected.txt
index 003fbf4..f7b83e9 100644 (file)
--- a/LayoutTests/inspector/debugger/async-stack-trace-expected.txt
+++ b/LayoutTests/inspector/debugger/async-stack-trace-expected.txt
@@ -1,4 +1,4 @@
-CONSOLE MESSAGE: line 55: Unable to post message to http://example.com. Recipient has origin .
+CONSOLE MESSAGE: line 83: Unable to post message to http://example.com. Recipient has origin .
 
 Tests for async stack traces.
 
@@ -80,6 +80,26 @@ ASYNC CALL STACK:
 5: [F] testAddEventListener
 6: [P] Global Code
 
+-- Running test case: CheckAsyncStackTrace.AddAttributeEventListener
+PAUSE #1
+CALL STACK:
+0: [F] pauseThenFinishTest
+1: [F] handleClick
+2: [F] testAttributeEventListener
+3: [P] Global Code
+4: [F] testAttributeEventListener
+5: [P] Global Code
+
+-- Running test case: CheckAsyncStackTrace.ReplaceAttributeEventListener
+PAUSE #1
+CALL STACK:
+0: [F] pauseThenFinishTest
+1: [F] handleClick2
+2: [F] testReplaceAttributeEventListener
+3: [P] Global Code
+4: [F] testReplaceAttributeEventListener
+5: [P] Global Code
+
 -- Running test case: CheckAsyncStackTrace.PostMessage
 PAUSE #1
 CALL STACK:

diff --git a/LayoutTests/inspector/debugger/async-stack-trace.html b/LayoutTests/inspector/debugger/async-stack-trace.html
index 13d83a1..0dea141 100644 (file)
--- a/LayoutTests/inspector/debugger/async-stack-trace.html
+++ b/LayoutTests/inspector/debugger/async-stack-trace.html
@@ -50,6 +50,34 @@ function testAddEventListener() {
     document.body.click();
 }
 
+function testAttributeEventListener() {
+    let previousListener = document.body.onclick;
+
+    function handleClick() {
+        document.body.onclick = previousListener;
+        pauseThenFinishTest();
+    }
+
+    document.body.onclick = handleClick;
+    document.body.click();
+}
+
+function testReplaceAttributeEventListener() {
+    let previousListener = document.body.onclick;
+
+    function handleClick1() {}
+
+    function handleClick2() {
+        document.body.onclick = previousListener;
+        pauseThenFinishTest();
+    }
+
+    document.body.onclick = handleClick1;
+    document.body.addEventListener("click", handleClick1, {once: true});
+    document.body.onclick = handleClick2;
+    document.body.click();
+}
+
 function testPostMessage(targetOrigin = "*") {
     let childFrame = document.getElementById("postMessageTestFrame");
     childFrame.contentWindow.postMessage("<message>", targetOrigin);
@@ -121,6 +149,11 @@ function test()
     addSimpleTestCase("ChainedRequestAnimationFrame", "testChainedRequestAnimationFrame()");
     addSimpleTestCase("ReferenceCounting", "testReferenceCounting()");
     addSimpleTestCase("AddEventListener", "testAddEventListener()");
+    // FIXME: <https://webkit.org/b/183236> Web Inspector: Async stack trace for an on-event handler doesn't include a boundary frame.
+    // Update test results once this has been addressed.
+    addSimpleTestCase("AddAttributeEventListener", "testAttributeEventListener()");
+    // Test for <https://webkit.org/b/181580> Web Inspector: ASSERT_NOT_REACHED in PageDebuggerAgent::didAddEventListener when page adds attribute event listener.
+    addSimpleTestCase("ReplaceAttributeEventListener", "testReplaceAttributeEventListener()");
     addSimpleTestCase("PostMessage", "testPostMessage()");
 
     suite.addTestCase({

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 8c7ef67..d8a5acb 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,30 @@
+2018-05-10  Matt Baker  <mattbaker@apple.com>
+
+        Web Inspector: ASSERT_NOT_REACHED in PageDebuggerAgent::didAddEventListener when page adds attribute event listener
+        https://bugs.webkit.org/show_bug.cgi?id=181580
+        <rdar://problem/36461309>
+
+        Reviewed by Brian Burg.
+
+        EventTarget should pass newly added EventListeners to InspectorInstrumentation,
+        instead of PageDebuggerAgent assuming the last item in the EventListenerVector
+        is the most recently added listener. This assumption does not hold when
+        the new listener replaces an existing listener.
+
+        * dom/EventTarget.cpp:
+        (WebCore::EventTarget::addEventListener):
+        (WebCore::EventTarget::setAttributeEventListener):
+
+        * inspector/InspectorInstrumentation.cpp:
+        (WebCore::InspectorInstrumentation::didAddEventListenerImpl):
+
+        * inspector/InspectorInstrumentation.h:
+        (WebCore::InspectorInstrumentation::didAddEventListener):
+
+        * inspector/agents/page/PageDebuggerAgent.cpp:
+        (WebCore::PageDebuggerAgent::didAddEventListener):
+        * inspector/agents/page/PageDebuggerAgent.h:
+
 2018-05-10  Chris Dumez  <cdumez@apple.com>
 
         'Cross-Origin-Options header implementation follow-up

diff --git a/Source/WebCore/dom/EventTarget.cpp b/Source/WebCore/dom/EventTarget.cpp
index 2f60c31..d52d355 100644 (file)
--- a/Source/WebCore/dom/EventTarget.cpp
+++ b/Source/WebCore/dom/EventTarget.cpp
@@ -75,12 +75,13 @@ bool EventTarget::addEventListener(const AtomicString& eventType, Ref<EventListe
     }
 
     bool listenerCreatedFromScript = listener->type() == EventListener::JSEventListenerType && !listener->wasCreatedFromMarkup();
+    auto listenerRef = listener.copyRef();
 
     if (!ensureEventTargetData().eventListenerMap.add(eventType, WTFMove(listener), { options.capture, passive.value_or(false), options.once }))
         return false;
 
     if (listenerCreatedFromScript)
-        InspectorInstrumentation::didAddEventListener(*this, eventType);
+        InspectorInstrumentation::didAddEventListener(*this, eventType, listenerRef.get(), options.capture);
 
     return true;
 }
@@ -135,9 +136,10 @@ bool EventTarget::setAttributeEventListener(const AtomicString& eventType, RefPt
     if (existingListener) {
         InspectorInstrumentation::willRemoveEventListener(*this, eventType, *existingListener, false);
 
+        auto listenerPointer = listener.copyRef();
         eventTargetData()->eventListenerMap.replace(eventType, *existingListener, listener.releaseNonNull(), { });
 
-        InspectorInstrumentation::didAddEventListener(*this, eventType);
+        InspectorInstrumentation::didAddEventListener(*this, eventType, *listenerPointer, false);
 
         return true;
     }

diff --git a/Source/WebCore/inspector/InspectorInstrumentation.cpp b/Source/WebCore/inspector/InspectorInstrumentation.cpp
index 3bd4bc0..5d3488c 100644 (file)
--- a/Source/WebCore/inspector/InspectorInstrumentation.cpp
+++ b/Source/WebCore/inspector/InspectorInstrumentation.cpp
@@ -313,10 +313,10 @@ void InspectorInstrumentation::didRemoveTimerImpl(InstrumentingAgents& instrumen
         timelineAgent->didRemoveTimer(timerId, frameForScriptExecutionContext(context));
 }
 
-void InspectorInstrumentation::didAddEventListenerImpl(InstrumentingAgents& instrumentingAgents, EventTarget& target, const AtomicString& eventType)
+void InspectorInstrumentation::didAddEventListenerImpl(InstrumentingAgents& instrumentingAgents, EventTarget& target, const AtomicString& eventType, EventListener& listener, bool capture)
 {
     if (PageDebuggerAgent* pageDebuggerAgent = instrumentingAgents.pageDebuggerAgent())
-        pageDebuggerAgent->didAddEventListener(target, eventType);
+        pageDebuggerAgent->didAddEventListener(target, eventType, listener, capture);
     if (InspectorDOMAgent* domAgent = instrumentingAgents.inspectorDOMAgent())
         domAgent->didAddEventListener(target);
 }

diff --git a/Source/WebCore/inspector/InspectorInstrumentation.h b/Source/WebCore/inspector/InspectorInstrumentation.h
index 8f93f45..6774fe0 100644 (file)
--- a/Source/WebCore/inspector/InspectorInstrumentation.h
+++ b/Source/WebCore/inspector/InspectorInstrumentation.h
@@ -146,7 +146,7 @@ public:
 
     static InspectorInstrumentationCookie willCallFunction(ScriptExecutionContext*, const String& scriptName, int scriptLine);
     static void didCallFunction(const InspectorInstrumentationCookie&, ScriptExecutionContext*);
-    static void didAddEventListener(EventTarget&, const AtomicString& eventType);
+    static void didAddEventListener(EventTarget&, const AtomicString& eventType, EventListener&, bool capture);
     static void willRemoveEventListener(EventTarget&, const AtomicString& eventType, EventListener&, bool capture);
     static bool isEventListenerDisabled(EventTarget&, const AtomicString& eventType, EventListener&, bool capture);
     static InspectorInstrumentationCookie willDispatchEvent(Document&, const Event&, bool hasEventListeners);
@@ -330,7 +330,7 @@ private:
 
     static InspectorInstrumentationCookie willCallFunctionImpl(InstrumentingAgents&, const String& scriptName, int scriptLine, ScriptExecutionContext*);
     static void didCallFunctionImpl(const InspectorInstrumentationCookie&, ScriptExecutionContext*);
-    static void didAddEventListenerImpl(InstrumentingAgents&, EventTarget&, const AtomicString& eventType);
+    static void didAddEventListenerImpl(InstrumentingAgents&, EventTarget&, const AtomicString& eventType, EventListener&, bool capture);
     static void willRemoveEventListenerImpl(InstrumentingAgents&, EventTarget&, const AtomicString& eventType, EventListener&, bool capture);
     static bool isEventListenerDisabledImpl(InstrumentingAgents&, EventTarget&, const AtomicString& eventType, EventListener&, bool capture);
     static InspectorInstrumentationCookie willDispatchEventImpl(InstrumentingAgents&, Document&, const Event&, bool hasEventListeners);
@@ -683,11 +683,11 @@ inline void InspectorInstrumentation::didRemoveTimer(ScriptExecutionContext& con
         didRemoveTimerImpl(*instrumentingAgents, timerId, context);
 }
 
-inline void InspectorInstrumentation::didAddEventListener(EventTarget& target, const AtomicString& eventType)
+inline void InspectorInstrumentation::didAddEventListener(EventTarget& target, const AtomicString& eventType, EventListener& listener, bool capture)
 {
     FAST_RETURN_IF_NO_FRONTENDS(void());
     if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsForContext(target.scriptExecutionContext()))
-        didAddEventListenerImpl(*instrumentingAgents, target, eventType);
+        didAddEventListenerImpl(*instrumentingAgents, target, eventType, listener, capture);
 }
 
 inline void InspectorInstrumentation::willRemoveEventListener(EventTarget& target, const AtomicString& eventType, EventListener& listener, bool capture)

diff --git a/Source/WebCore/inspector/agents/page/PageDebuggerAgent.cpp b/Source/WebCore/inspector/agents/page/PageDebuggerAgent.cpp
index 29cdda2..75508e5 100644 (file)
--- a/Source/WebCore/inspector/agents/page/PageDebuggerAgent.cpp
+++ b/Source/WebCore/inspector/agents/page/PageDebuggerAgent.cpp
@@ -164,14 +164,20 @@ void PageDebuggerAgent::mainFrameNavigated()
     setSuppressAllPauses(false);
 }
 
-void PageDebuggerAgent::didAddEventListener(EventTarget& target, const AtomicString& eventType)
+void PageDebuggerAgent::didAddEventListener(EventTarget& target, const AtomicString& eventType, EventListener& listener, bool capture)
 {
     if (!breakpointsActive())
         return;
 
     auto& eventListeners = target.eventListeners(eventType);
-    const RefPtr<RegisteredEventListener>& listener = eventListeners.last();
-    if (m_registeredEventListeners.contains(listener.get())) {
+    auto position = eventListeners.findMatching([&](auto& registeredListener) {
+        return &registeredListener->callback() == &listener && registeredListener->useCapture() == capture;
+    });
+    if (position == notFound)
+        return;
+
+    auto& registeredListener = eventListeners.at(position);
+    if (m_registeredEventListeners.contains(registeredListener.get())) {
         ASSERT_NOT_REACHED();
         return;
     }
@@ -181,9 +187,9 @@ void PageDebuggerAgent::didAddEventListener(EventTarget& target, const AtomicStr
         return;
 
     int identifier = m_nextEventListenerIdentifier++;
-    m_registeredEventListeners.set(listener.get(), identifier);
+    m_registeredEventListeners.set(registeredListener.get(), identifier);
 
-    didScheduleAsyncCall(scriptState, InspectorDebuggerAgent::AsyncCallType::EventListener, identifier, listener->isOnce());
+    didScheduleAsyncCall(scriptState, InspectorDebuggerAgent::AsyncCallType::EventListener, identifier, registeredListener->isOnce());
 }
 
 void PageDebuggerAgent::willRemoveEventListener(EventTarget& target, const AtomicString& eventType, EventListener& listener, bool capture)

diff --git a/Source/WebCore/inspector/agents/page/PageDebuggerAgent.h b/Source/WebCore/inspector/agents/page/PageDebuggerAgent.h
index d2b8b89..8943d9a 100644 (file)
--- a/Source/WebCore/inspector/agents/page/PageDebuggerAgent.h
+++ b/Source/WebCore/inspector/agents/page/PageDebuggerAgent.h
@@ -61,7 +61,7 @@ public:
     void willFireAnimationFrame(int callbackId);
     void didCancelAnimationFrame(int callbackId);
 
-    void didAddEventListener(EventTarget&, const AtomicString& eventType);
+    void didAddEventListener(EventTarget&, const AtomicString& eventType, EventListener&, bool capture);
     void willRemoveEventListener(EventTarget&, const AtomicString& eventType, EventListener&, bool capture);
     void willHandleEvent(const RegisteredEventListener&);