Bug 209862

Summary: ASSERTION FAILED: m_wrapper on fast/events/scoped/editing-commands.html
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: WebCore JavaScriptAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, darin, esprehn+autocc, ews-watchlist, ggaren, kangil.han, rniwa, sihui_liu, webkit-bot-watchers-bugzilla, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Alexey Proskuryakov 2020-04-01 10:43:22 PDT 
I get a reproducible assertion failure on fast/events/scoped/editing-commands.html.

run-webkit-tests -v --repeat-each 10 -1 --additional-env-var=JSC_slowPathAllocsBetweenGCs=25 LayoutTests/fast/events/scoped/editing-commands.html

ASSERTION FAILED: m_wrapper
./bindings/js/JSEventListener.h(125) : JSC::JSObject *WebCore::JSEventListener::ensureJSFunction(WebCore::ScriptExecutionContext &) const
1   0x110217779 WTFCrash
2   0x12804603b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x12a24c76f WebCore::JSEventListener::ensureJSFunction(WebCore::ScriptExecutionContext&) const
4   0x12a24badd WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext&, WebCore::Event&)
5   0x12a898107 WebCore::EventTarget::innerInvokeEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::DumbPtrTraits<WebCore::RegisteredEventListener> >, 1ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>, WebCore::EventTarget::EventInvokePhase)
6   0x12a894364 WebCore::EventTarget::fireEventListeners(WebCore::Event&, WebCore::EventTarget::EventInvokePhase)
7   0x12a90a5a2 WebCore::Node::handleLocalEvents(WebCore::Event&, WebCore::EventTarget::EventInvokePhase)
8   0x12a882bf1 WebCore::EventContext::handleLocalEvents(WebCore::Event&, WebCore::EventTarget::EventInvokePhase) const
9   0x12a8836bf WebCore::dispatchEventInDOM(WebCore::Event&, WebCore::EventPath const&)
10  0x12a8831f7 WebCore::EventDispatcher::dispatchEvent(WebCore::Node&, WebCore::Event&)
11  0x12a90a5fd WebCore::Node::dispatchEvent(WebCore::Event&)
12  0x12a952b9e WebCore::ScopedEventQueue::dispatchEvent(WebCore::Event&) const
13  0x12a952c25 WebCore::ScopedEventQueue::dispatchAllEvents()
14  0x12a952ddd WebCore::ScopedEventQueue::decrementScopingLevel()
15  0x12a7ba709 WebCore::EventQueueScope::~EventQueueScope()
16  0x12a782d15 WebCore::EventQueueScope::~EventQueueScope()
17  0x12a7a0daa WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&)
18  0x1288fc9e4 WebCore::jsDocumentPrototypeFunctionExecCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*, JSC::ThrowScope&)
19  0x128802d72 long long WebCore::IDLOperation<WebCore::JSDocument>::call<&(WebCore::jsDocumentPrototypeFunctionExecCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*)
20  0x128802a54 WebCore::jsDocumentPrototypeFunctionExecCommand(JSC::JSGlobalObject*, JSC::CallFrame*)
21  0x4f297d401178
22  0x1107185b9 llint_entry
23  0x1106fb233 vmEntryToJavaScript
24  0x111484957 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
25  0x111483f34 JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::JSGlobalObject*, JSC::JSObject*)
26  0x1117ecc9c JSC::evaluate(JSC::JSGlobalObject*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
27  0x1117ece48 JSC::profiledEvaluate(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
28  0x12a2d0018 WebCore::JSExecState::profiledEvaluate(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
29  0x12a2cfc67 WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&)
30  0x12a2cfab9 WebCore::ScriptController::evaluateInWorldIgnoringException(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&)
31  0x12a2d02f5 WebCore::ScriptController::evaluateIgnoringException(WebCore::ScriptSourceCode const&)
Comment 1 Radar WebKit Bug Importer 2020-04-01 10:43:33 PDT 
<rdar://problem/61164607>
Comment 2 Alexey Proskuryakov 2020-04-01 11:01:20 PDT 
r259292
Comment 3 Chris Dumez 2020-05-20 12:51:24 PDT 
Created attachment 399876 [details]
Patch
Comment 4 Sihui Liu 2020-05-20 16:18:54 PDT 
Comment on attachment 399876 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        so that it keeps alive alive both the target and its JS wrapper.

double "alive"

> Source/WebCore/dom/ScopedEventQueue.cpp:64
> +    Vector<ScopedEvent> queuedEvents = WTFMove(m_queuedEvents);

Is there a difference between releasing wrapper after all events are dispatched and releasing wrapper after each event is dispatched?
Comment 5 Chris Dumez 2020-05-21 09:09:48 PDT 
Created attachment 399959 [details]
Patch
Comment 6 Darin Adler 2020-05-21 09:47:34 PDT 
Comment on attachment 399959 [details]
Patch

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

> Source/WebCore/dom/ScopedEventQueue.cpp:50
> +    ScopedEvent scopedEvent = { WTFMove(event), target };

3 ways to write this:

1: ScopedEvent scopedEvent { WTFMove(event), target };
2: ScopedEvent scopedEvent = { WTFMove(event), target };
3: auto scopedEvent = ScopedEvent { WTFMove(event), target };

Style (1) seems like the most "normal". Antti convinced me to use style (3) in new WebKit code I am writing.

> Source/WebCore/dom/ScopedEventQueue.cpp:64
> +    Vector<ScopedEvent> queuedEvents = WTFMove(m_queuedEvents);

I suggest using auto here.

> Source/WebCore/dom/ScopedEventQueue.h:59
> +    void dispatchEvent(ScopedEvent&) const;

Why does this take a reference? Is this an in/out in some way?
Comment 7 Chris Dumez 2020-05-21 09:50:17 PDT 
Comment on attachment 399959 [details]
Patch

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

>> Source/WebCore/dom/ScopedEventQueue.cpp:64
>> +    Vector<ScopedEvent> queuedEvents = WTFMove(m_queuedEvents);
> 
> I suggest using auto here.

Technically, based on your recent comments, it is supposed to use std::exchange() instead of WTFMove() too since m_queuedEvents is reused after that.
Comment 8 Darin Adler 2020-05-21 09:52:46 PDT 
Comment on attachment 399959 [details]
Patch

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

>>> Source/WebCore/dom/ScopedEventQueue.cpp:64
>>> +    Vector<ScopedEvent> queuedEvents = WTFMove(m_queuedEvents);
>> 
>> I suggest using auto here.
> 
> Technically, based on your recent comments, it is supposed to use std::exchange() instead of WTFMove() too since m_queuedEvents is reused after that.

Good point!
Comment 9 Chris Dumez 2020-05-21 10:04:16 PDT 
Created attachment 399961 [details]
Patch
Comment 10 EWS 2020-05-21 11:03:41 PDT 
Committed r262016: <https://trac.webkit.org/changeset/262016>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399961 [details].