Attachment 339761 Details for Bug 181580 – Patch

[patch] Patch

bug-181580-20180507154705.patch (text/plain), 14.54 KB, created by Matt Baker on 2018-05-07 15:47:06 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Matt Baker

Created: 2018-05-07 15:47:06 PDT

Size: 14.54 KB

patch

obsolete

>Subversion Revision: 231451
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index fd142b5eb23aa67dab46fe5a586368ba2f066edb..de8e939501ed3f7d80fc85dfd4f56308629c7827 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,30 @@
>+2018-05-07  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 NOBODY (OOPS!).
>+
>+        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-07  Chris Dumez  <cdumez@apple.com>
> 
>         ASSERT(!childItemWithTarget(child->target())) is hit in HistoryItem::addChildItem()
>diff --git a/Source/WebCore/dom/EventTarget.cpp b/Source/WebCore/dom/EventTarget.cpp
>index 2f60c319298ed25e2ce893e67670d97c5207e765..d52d35547ce282e9b265acf10170a37b518647a8 100644
>--- 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 3bd4bc0a48fa8b6f75f29a10de457ed0e19689cc..5d3488c9feadf70a00e800e0e394e49d2880a750 100644
>--- 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 8f93f4537fc2150ea7d80cf4df83a51ee3c186aa..6774fe0b72bb0b9d31d9e059d20055fd4e9f3653 100644
>--- 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 29cdda29ee4c0d4f9cb07a4085d2b1add56b628e..75508e5ebbc0f62b0691d5815d0386e95775202b 100644
>--- 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 d2b8b89a4afc239177f8a3af86a9d9bdc6a882f0..8943d9ad5cb9b85c9e766da6b4340018b1189388 100644
>--- 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&);
> 
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 1a0838edc704f3ebe419d4405305c9526f27204c..3f7a8d94ae37e478fe7303284e8f201cbe99e9e2 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,17 @@
>+2018-05-07  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 NOBODY (OOPS!).
>+
>+        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-07  Chris Dumez  <cdumez@apple.com>
> 
>         ASSERT(!childItemWithTarget(child->target())) is hit in HistoryItem::addChildItem()
>diff --git a/LayoutTests/inspector/debugger/async-stack-trace-expected.txt b/LayoutTests/inspector/debugger/async-stack-trace-expected.txt
>index 003fbf462620adb857d6aeb7641a8967fcdf5657..f7b83e9b6db2bd5f981d2adca5d0e12be1fe6058 100644
>--- 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 13d83a1b01dc6fd477403d3b9767941704222095..0dea141f8072cc833383f3b98989f3a871a2f516 100644
>--- 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({

Actions: View | Formatted Diff | Diff

Attachments on bug 181580: 334793 | 335204 | 335212 | 335213 | 335217 | 335219 | 338813 | 338829 | 338843 | 338848 | 339265 | 339266 | 339480 | 339761