Bug 209862 – ASSERTION FAILED: m_wrapper on fast/events/scoped/editing-commands.html

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

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&)

[Radar WebKit Bug Importer]

<rdar://problem/61164607>

[Alexey Proskuryakov]

r259292

[Chris Dumez]

Created attachment 399876 [details]
Patch

[Sihui Liu]

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?

[Chris Dumez]

Created attachment 399959 [details]
Patch

[Darin Adler]

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?

[Chris Dumez]

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.

[Darin Adler]

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!

[Chris Dumez]

Created attachment 399961 [details]
Patch

[EWS]

Committed r262016: <https://trac.webkit.org/changeset/262016>

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