Bug 173540 – AX: Layout test imported/w3c/web-platform-tests/html/syntax/parsing/html5lib_tests1.html flaky crash

• Component: Accessibility
• Status: RESOLVED FIXED

[Fujii Hironori]

[GTK] Layout Test imported/w3c/web-platform-tests/html/syntax/parsing/html5lib_tests1.html flaky crash

GTK Linux 64-bit Release

> Thread 1 (Thread 0x7f22c4665f00 (LWP 19353)):
> #0  0x00007f22d744b174 in WebCore::AXObjectCache::performDeferredCacheUpdate() ()
> #1  0x00007f22d7baa044 in WebCore::FrameView::layout(bool) ()
> #2  0x00007f22d7baa7f1 in WebCore::FrameView::updateContentsSize() ()
> #3  0x00007f22d7c62366 in WebCore::ScrollView::updateScrollbars(WebCore::IntPoint const&) ()
> #4  0x00007f22d7c62b17 in WebCore::ScrollView::setContentsSize(WebCore::IntSize const&) ()
> #5  0x00007f22d7ba8606 in WebCore::FrameView::setContentsSize(WebCore::IntSize const&) ()
> #6  0x00007f22d7ba8764 in WebCore::FrameView::adjustViewSize() ()
> #7  0x00007f22d7baa0f6 in WebCore::FrameView::layout(bool) ()
> #8  0x00007f22d7c6f80a in WebCore::ThreadTimers::sharedTimerFiredInternal() ()
> #9  0x00007f22d1f9b3ba in WTF::RunLoop::TimerBase::TimerBase(WTF::RunLoop&)::{lambda(void*)#1}::_FUN(void*) () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
> #10 0x00007f22cfcd05ca in g_main_dispatch () at /home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/DependenciesGTK/Source/glib-2.52.1/glib/gmain.c:3212
> #11 g_main_context_dispatch () at /home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/DependenciesGTK/Source/glib-2.52.1/glib/gmain.c:3865
> #12 0x00007f22cfcd0948 in g_main_context_iterate () at /home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/DependenciesGTK/Source/glib-2.52.1/glib/gmain.c:3938
> #13 0x00007f22cfcd0c62 in g_main_loop_run () at /home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/DependenciesGTK/Source/glib-2.52.1/glib/gmain.c:4134
> #14 0x00007f22d1f9b7b0 in WTF::RunLoop::run() () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
> #15 0x00007f22d43c9542 in int WebKit::ChildProcessMain<WebKit::WebProcess, WebKit::WebProcessMain>(int, char**) () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #16 0x00007f22cb6032b1 in __libc_start_main (main=0x7f22d729f590 <main>, argc=2, argv=0x7ffcd0f9ca38, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7ffcd0f9ca28) at ../csu/libc-start.c:291
> #17 0x00007f22d729faaa in _start ()

[Fujii Hironori]

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

[Fujii Hironori]

In AXObjectCache::performDeferredCacheUpdate:

>    for (auto* element : m_deferredRecomputeIsIgnoredList) {
>        if (auto* renderer = element->renderer())

element was already destructed.

[Fujii Hironori]

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

[Radar WebKit Bug Importer]

<rdar://problem/35386393>

[Fujii Hironori]

Created attachment 326319 [details]
WIP patch

[Fujii Hironori]

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.

[Fujii Hironori]

Created attachment 326429 [details]
WIP patch

[Fujii Hironori]

Created attachment 328334 [details]
Patch

[Michael Catanzaro]

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

[Michael Catanzaro]

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

[zalan]

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.

[chris fleizach]

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

[zalan]

(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?

[zalan]

(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.

[Michael Catanzaro]

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

[Fujii Hironori]

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

[Fujii Hironori]

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

[zalan]

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!

[Fujii Hironori]

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.

[zalan]

(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.

[WebKit Commit Bot]

Comment on attachment 328670 [details]
Unreviewed gardening patch

Clearing flags on attachment: 328670

Committed r225614: <https://trac.webkit.org/changeset/225614>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.