Web Inspector: ASSERT_NOT_REACHED hit in PageDebuggerAgent::didAddEventListener

[Ross Kirsling]

Repro steps:

1. run-minibrowser --debug / run-safari --debug
2. Open Web Inspector.
3. Enter "playstation.com" into URL bar.

Results:

1. this.treeOutline is unexpectedly null @ https://trac.webkit.org/browser/webkit/trunk/Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js#L1340
2. "SHOULD NEVER BE REACHED" @ https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/inspector/PageDebuggerAgent.cpp#L174

Note:

- Adding a null-check in DOMTreeElement does not stop the WebCore crash in PageDebuggerAgent.
- I assume this is related to https://trac.webkit.org/changeset/220036/webkit (unless result 1 points to a different root cause that I haven't identified).

[Ross Kirsling]

Result (1) mentioned above is unrelated and addressed in https://bugs.webkit.org/show_bug.cgi?id=178745.

[Joseph Pecoraro]

> void PageDebuggerAgent::didAddEventListener(EventTarget& target, const AtomicString& eventType)
> {
>     ...
>     auto& eventListeners = target.eventListeners(eventType);
>     const RefPtr<RegisteredEventListener>& listener = eventListeners.last();
>     if (m_registeredEventListeners.contains(listener.get())) {
>         ASSERT_NOT_REACHED();
>         return;
>     }
>     ...
>

So this looks like its getting a didAddEventListener twice for the same event. That or the eventListeners.last() is a poor choice. Do you have a backtrace to see how we got here? I'd be interested to know what EventTarget entry point got us here.

[Ross Kirsling]

It really is as easy as visiting playstation.com, but here you go. :P
> 1   0x119c30c7d WTFCrash
> 2   0x10d325c66 WebCore::PageDebuggerAgent::didAddEventListener(WebCore::EventTarget&, WTF::AtomicString const&)
> 3   0x10cfe0d7d WebCore::InspectorInstrumentation::didAddEventListenerImpl(WebCore::InstrumentingAgents&, WebCore::EventTarget&, WTF::AtomicString const&)
> 4   0x10f1e5152 WebCore::InspectorInstrumentation::didAddEventListener(WebCore::EventTarget&, WTF::AtomicString const&)
> 5   0x10f1e576c WebCore::EventTarget::setAttributeEventListener(WTF::AtomicString const&, WTF::RefPtr<WebCore::EventListener>&&, WebCore::DOMWrapperWorld&)
> 6   0x10ed2b214 WebCore::setEventHandlerAttribute(JSC::ExecState&, JSC::JSObject&, WebCore::EventTarget&, WTF::AtomicString const&, JSC::JSValue)
> 7   0x10de6b079 WebCore::setJSDOMWindowOnloadSetter(JSC::ExecState&, WebCore::JSDOMWindow&, JSC::JSValue, JSC::ThrowScope&)
> 8   0x10ddeaad8 bool WebCore::IDLAttribute<WebCore::JSDOMWindow>::set<&(WebCore::setJSDOMWindowOnloadSetter(JSC::ExecState&, WebCore::JSDOMWindow&, JSC::JSValue, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::ExecState&, long long, long long, char const*)
> 9   0x10ddea99c WebCore::setJSDOMWindowOnload(JSC::ExecState*, long long, long long)
> 10  0x11976b82d JSC::callCustomSetter(JSC::ExecState*, bool (*)(JSC::ExecState*, long long, long long), bool, JSC::JSValue, JSC::JSValue)
> 11  0x1198606bc JSC::putEntry(JSC::ExecState*, JSC::ClassInfo const*, JSC::HashTableValue const*, JSC::JSObject*, JSC::JSObject*, JSC::PropertyName, JSC::JSValue, JSC::PutPropertySlot&)
> 12  0x11985fd86 JSC::JSObject::putInlineSlow(JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::PutPropertySlot&)
> 13  0x11904b5a0 JSC::JSObject::putInlineForJSObject(JSC::JSCell*, JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::PutPropertySlot&)
> 14  0x119850925 JSC::JSObject::put(JSC::JSCell*, JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::PutPropertySlot&)
> 15  0x1197f7d73 JSC::JSGlobalObject::put(JSC::JSCell*, JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::PutPropertySlot&)
> 16  0x10ed16ebc WebCore::JSDOMWindow::put(JSC::JSCell*, JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::PutPropertySlot&)
> 17  0x1198842e5 JSC::JSProxy::put(JSC::JSCell*, JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::PutPropertySlot&)
> 18  0x11904b0fc JSC::JSCell::putInline(JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::PutPropertySlot&)
> 19  0x11904c803 JSC::JSValue::putInline(JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::PutPropertySlot&)
> 20  0x11957bf62 llint_slow_path_put_by_id
> 21  0x1187ebfcc llint_entry
> 22  0x1187e8477 vmEntryToJavaScript
> 23  0x1195106de JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
> 24  0x1194b71e6 JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*)
> 25  0x119732297 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
> 26  0x119732420 JSC::profiledEvaluate(JSC::ExecState*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
> 27  0x10ed5bbdb WebCore::JSMainThreadExecState::profiledEvaluate(JSC::ExecState*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
> 28  0x10ed5b9c8 WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&, WebCore::ExceptionDetails*)
> 29  0x10ed5bcbd WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&, WebCore::ExceptionDetails*)
> 30  0x10f260bb2 WebCore::ScriptElement::executeClassicScript(WebCore::ScriptSourceCode const&)
> 31  0x10f25f156 WebCore::ScriptElement::prepareScript(WTF::TextPosition const&, WebCore::ScriptElement::LegacyTypeSupport)

[Joseph Pecoraro]

> So this looks like its getting a didAddEventListener twice for the same
> event. That or the eventListeners.last() is a poor choice. Do you have a
> backtrace to see how we got here? I'd be interested to know what EventTarget
> entry point got us here.

Yeah, I think last() is the wrong choice.

>    bool EventTarget::setAttributeEventListener(const AtomicString& eventType, RefPtr<EventListener>&& listener, DOMWrapperWorld& isolatedWorld)
>    {
>        auto* existingListener = attributeEventListener(eventType, isolatedWorld);
>        ...
>        if (existingListener) {
>            InspectorInstrumentation::willRemoveEventListener(*this, eventType, *existingListener, false);
>            eventTargetData()->eventListenerMap.replace(eventType, *existingListener, listener.releaseNonNull(), { });
>            InspectorInstrumentation::didAddEventListener(*this, eventType);
>            return true;
>        }
>        ...
>    }

This has an eventListenerMap.replace(), probably means that when didAddEventListener gets call, "last()" might not be the right value to use.

[Ross Kirsling]

Created attachment 339247 [details]
Patch

[Ross Kirsling]

Comment on attachment 339247 [details]
Patch

This still needs a test, but I wanted to get a rough draft solution up in the meantime.

[EWS Watchlist]

Attachment 339247 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Matt Baker]

Is this a different case than that covered by https://webkit.org/b/181580?

[Ross Kirsling]

(In reply to Matt Baker from comment #8)
> Is this a different case than that covered by https://webkit.org/b/181580?

Looks to be one and the same! Hehe.

[Ross Kirsling]

*** This bug has been marked as a duplicate of bug 181580 ***