Bug 173540

*** Bug 173957 has been marked as a duplicate of this bug. ***

In AXObjectCache::performDeferredCacheUpdate: > for (auto* element : m_deferredRecomputeIsIgnoredList) { > if (auto* renderer = element->renderer()) element was already destructed.

Created attachment 326199 [details] debug patch to detect an element is destructed without unregistering from m_deferredRecomputeIsIgnoredList I created a debug patch to detect an element is destructed without unregistering from m_deferredRecomputeIsIgnoredList. And, I got a following bt. It shows a body element was destructed without unregistering from m_deferredRecomputeIsIgnoredList. > (gdb) bt > #0 0x00007fe7ce43232c in WTFCrash () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18 > #1 0x00007fe7d356e5f5 in WebCore::Node::~Node() () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > #2 0x00007fe7d367c2e9 in WebCore::HTMLBodyElement::~HTMLBodyElement() () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > #3 0x00007fe7d34d97dc in WebCore::ChildNodeList::~ChildNodeList() () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > #4 0x00007fe7d34d9809 in WebCore::ChildNodeList::~ChildNodeList() () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > #5 0x00007fe7ce1f53ae in JSC::JSDestructibleObjectSubspace::finishSweep(JSC::MarkedBlock::Handle&, JSC::FreeList*) () > from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18 > #6 0x00007fe7cdeda5a3 in JSC::MarkedBlock::Handle::sweep(JSC::FreeList*) () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18 > #7 0x00007fe7cdedb8b1 in JSC::MarkedAllocator::tryAllocateWithoutCollecting() () > from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18 > #8 0x00007fe7cdedbf25 in JSC::MarkedAllocator::allocateSlowCaseImpl(JSC::GCDeferralContext*, bool) () > from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18 > #9 0x00007fe7ce1dd807 in JSC::Structure::create(JSC::VM&, JSC::JSGlobalObject*, JSC::JSValue, JSC::TypeInfo const&, JSC::ClassInfo const*, unsigned char, unsigned int) [clone .constprop.627] () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18 > #10 0x00007fe7ce1f8852 in JSC::JSGlobalObject::init(JSC::VM&) () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18 > #11 0x00007fe7ce2012e4 in JSC::JSGlobalObject::finishCreation(JSC::VM&, JSC::JSObject*) () > from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18 > #12 0x00007fe7d3324371 in WebCore::JSDOMGlobalObject::finishCreation(JSC::VM&, JSC::JSObject*) () > from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > #13 0x00007fe7d3327381 in WebCore::JSDOMWindowBase::finishCreation(JSC::VM&, WebCore::JSDOMWindowProxy*) () > from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > #14 0x00007fe7d2d1b053 in WebCore::JSDOMWindow::finishCreation(JSC::VM&, WebCore::JSDOMWindowProxy*) () > from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > #15 0x00007fe7d3331d5a in WebCore::JSDOMWindowProxy::setWindow(WTF::RefPtr<WebCore::DOMWindow>&&) () > from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > #16 0x00007fe7d3353117 in WebCore::ScriptController::setDOMWindowForWindowProxy(WebCore::DOMWindow*) () > from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > #17 0x00007fe7d381bdc6 in WebCore::FrameLoader::clear(WebCore::Document*, bool, bool, bool) () > from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > #18 0x00007fe7d3802019 in WebCore::DocumentWriter::begin(WebCore::URL const&, bool, WebCore::Document*) () > from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > #19 0x00007fe7d38023cd in WebCore::DocumentLoader::commitData(char const*, unsigned long) () > from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > #20 0x00007fe7d2a445fe in WebKit::WebFrameLoaderClient::committedLoad(WebCore::DocumentLoader*, char const*, int) () > from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > #21 0x00007fe7d3800986 in WebCore::DocumentLoader::commitLoad(char const*, int) () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > #22 0x00007fe7d388640a in WebCore::CachedRawResource::notifyClientsDataWasReceived(char const*, unsigned int) () > from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > #23 0x00007fe7d38865b5 in WebCore::CachedRawResource::updateBuffer(WebCore::SharedBuffer&) () > from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > #24 0x00007fe7d3850a81 in WebCore::SubresourceLoader::didReceiveDataOrBuffer(char const*, int, WTF::RefPtr<WebCore::SharedBuffer>&&, long long, WebCore::DataPayloadType) () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > #25 0x00007fe7d3850c25 in WebCore::SubresourceLoader::didReceiveData(char const*, unsigned int, long long, WebCore::DataPayloadType) () > from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > #26 0x00007fe7d2cad74c in WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::Decoder&) () > from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > #27 0x00007fe7d281fb0b in IPC::Connection::dispatchMessage(std::unique_ptr<IPC::Decoder, std::default_delete<IPC::Decoder> >) () > from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > #28 0x00007fe7d28204bd in IPC::Connection::dispatchOneMessage() () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > #29 0x00007fe7ce449d26 in WTF::RunLoop::performWork() () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18 > #30 0x00007fe7ce480c99 in WTF::RunLoop::RunLoop()::{lambda(void*)#1}::_FUN(void*) () > from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18 > #31 0x00007fe7ce947935 in g_main_dispatch () at /home/fujii/work/webkit/gb/WebKitBuild/DependenciesGTK/Source/glib-2.52.1/glib/gmain.c:3212 > #32 g_main_context_dispatch () at /home/fujii/work/webkit/gb/WebKitBuild/DependenciesGTK/Source/glib-2.52.1/glib/gmain.c:3865 > #33 0x00007fe7ce947cf8 in g_main_context_iterate () at /home/fujii/work/webkit/gb/WebKitBuild/DependenciesGTK/Source/glib-2.52.1/glib/gmain.c:3938 > #34 0x00007fe7ce948012 in g_main_loop_run () at /home/fujii/work/webkit/gb/WebKitBuild/DependenciesGTK/Source/glib-2.52.1/glib/gmain.c:4134 > #35 0x00007fe7ce481690 in WTF::RunLoop::run() () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18 > #36 0x00007fe7d2c27d98 in WebProcessMainUnix () from /home/fujii/work/webkit/gb/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > #37 0x00007fe7d0eb11c1 in __libc_start_main (main=0x56139b0c48c0 <main>, argc=2, argv=0x7fffa46edfa8, init=<optimized out>, fini=<optimized out>, > rtld_fini=<optimized out>, stack_end=0x7fffa46edf98) at ../csu/libc-start.c:308 > #38 0x000056139b0c494a in _start () > (gdb) q

<rdar://problem/35386393>

Created attachment 326319 [details] WIP patch

Comment on attachment 326319 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=326319&action=review > Source/WebCore/accessibility/AXObjectCache.cpp:119 > + // ListHashSet::remove(const ValueType&) can't be used because item->m_deletionHasBegun would be true. Too tricky. If a node is stored in ListHashSet<RefPtr<T>>, Node destructor never be called. If Node destructor is called, it means the instance is not stored in the ListHashSets. It doesn't need to remove this pointer within Node destructor.

Created attachment 326429 [details] WIP patch

Created attachment 328334 [details] Patch

Comment on attachment 328334 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328334&action=review > Source/WebCore/ChangeLog:3 > + [GTK] Layout Test imported/w3c/web-platform-tests/html/syntax/parsing/html5lib_tests1.html flaky crash Remove the [GTK] prefix since this is a platform-agnostic change

Chris F., do you want to review this one?

Comment on attachment 328334 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328334&action=review > Source/WebCore/accessibility/AXObjectCache.cpp:2753 > + m_deferredRecomputeIsIgnoredList.remove(element); I was under the impression that any node detaching/destruction would go through AXObjectCache::remove(). -apparently that's not the case. Chris, could you confirm that and let me know if there are other entry points in AXObjectCache for node detaching/destruction? Regardless, this patch introduces yet another cleanup path for the deferred objects which is highly error prone. I'd rather have them all in one function.

Comment on attachment 328334 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328334&action=review >> Source/WebCore/accessibility/AXObjectCache.cpp:2753 >> + m_deferredRecomputeIsIgnoredList.remove(element); > > I was under the impression that any node detaching/destruction would go through AXObjectCache::remove(). -apparently that's not the case. Chris, could you confirm that and let me know if there are other entry points in AXObjectCache for node detaching/destruction? > Regardless, this patch introduces yet another cleanup path for the deferred objects which is highly error prone. I'd rather have them all in one function. this is the only case. I don't think this list needs to strongly retain Nodes. I believe we're just doing pointer comparison here, so maybe this can be cleaned up to use a zero-referencing data structure and automatically clean itself up

(In reply to chris fleizach from comment #12) > Comment on attachment 328334 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=328334&action=review > > >> Source/WebCore/accessibility/AXObjectCache.cpp:2753 > >> + m_deferredRecomputeIsIgnoredList.remove(element); > > > > I was under the impression that any node detaching/destruction would go through AXObjectCache::remove(). -apparently that's not the case. Chris, could you confirm that and let me know if there are other entry points in AXObjectCache for node detaching/destruction? > > Regardless, this patch introduces yet another cleanup path for the deferred objects which is highly error prone. I'd rather have them all in one function. > > this is the only case. I don't think this list needs to strongly retain > Nodes. I believe we're just doing pointer comparison here, so maybe this can > be cleaned up to use a zero-referencing data structure and automatically > clean itself up Ok, thanks for confirming it. Fujii, do you mind if I take this over from you?

(In reply to zalan from comment #13) > (In reply to chris fleizach from comment #12) > > Comment on attachment 328334 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=328334&action=review > > > > >> Source/WebCore/accessibility/AXObjectCache.cpp:2753 > > >> + m_deferredRecomputeIsIgnoredList.remove(element); > > > > > > I was under the impression that any node detaching/destruction would go through AXObjectCache::remove(). -apparently that's not the case. Chris, could you confirm that and let me know if there are other entry points in AXObjectCache for node detaching/destruction? > > > Regardless, this patch introduces yet another cleanup path for the deferred objects which is highly error prone. I'd rather have them all in one function. > > > > this is the only case. I don't think this list needs to strongly retain > > Nodes. I believe we're just doing pointer comparison here, so maybe this can > > be cleaned up to use a zero-referencing data structure and automatically > > clean itself up > Ok, thanks for confirming it. Fujii, do you mind if I take this over from > you? Oh I just noticed that you've been working on this for a while now. Let me know if you'd like to finish it up.

I'm adding a crash expectation for this test, so please remove the expectation when landing.

GTK port is eager to create AXObjects. If a following patch is applied to disable GTK port specific code, GTK port doesn't crash. I guess this is the reason why other port doesn't crash. > diff --git a/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp b/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp > index 7866a77e832..a2c61510d40 100644 > --- a/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp > +++ b/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp > @@ -1678,7 +1678,7 @@ void WebFrameLoaderClient::dispatchDidClearWindowObjectInWorld(DOMWrapperWorld& > if (automationSessionProxy && world.isNormal()) > automationSessionProxy->didClearWindowObjectForFrame(*m_frame); > > -#if HAVE(ACCESSIBILITY) && PLATFORM(GTK) > +#if HAVE(ACCESSIBILITY) && PLATFORM(GTK) && 0 > // Ensure the accessibility hierarchy is updated. > webPage->updateAccessibilityTree(); > #endif

Anyway, I don't understand AX at all. Feel free to take over this bug. Thanks in advance.

This should be fixed by https://trac.webkit.org/changeset/225613/webkit I can't verify it though. Fuji, could you let me know if r225613 actually fixed the flaky test, please? Thanks!

Created attachment 328670 [details] Unreviewed gardening patch I've confirmed the flaky crash bug doesn't happen in r225613. Your patch looks way better than mine. Thank you.

(In reply to Fujii Hironori from comment #19) > Created attachment 328670 [details] > Unreviewed gardening patch > > I've confirmed the flaky crash bug doesn't happen in r225613. > Your patch looks way better than mine. Thank you. Thanks for investigating it! It helped a lot.

Comment on attachment 328670 [details] Unreviewed gardening patch Clearing flags on attachment: 328670 Committed r225614: <https://trac.webkit.org/changeset/225614>

All reviewed patches have been landed. Closing bug.