Bug 173540 - AX: Layout test imported/w3c/web-platform-tests/html/syntax/parsing/html5lib_tests1.html flaky crash
Summary: AX: Layout test imported/w3c/web-platform-tests/html/syntax/parsing/html5lib_...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar
: 173957 (view as bug list)
Depends on: 180503
Blocks:
  Show dependency treegraph
 
Reported: 2017-06-19 00:45 PDT by Fujii Hironori
Modified: 2017-12-06 19:23 PST (History)
17 users (show)

See Also:


Attachments
debug patch to detect an element is destructed without unregistering from m_deferredRecomputeIsIgnoredList (2.18 KB, patch)
2017-11-07 00:36 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (3.45 KB, patch)
2017-11-08 03:02 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (3.54 KB, patch)
2017-11-08 21:52 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (2.02 KB, patch)
2017-12-04 01:50 PST, Fujii Hironori
zalan: review-
zalan: commit-queue-
Details | Formatted Diff | Diff
Unreviewed gardening patch (1.48 KB, patch)
2017-12-06 19:01 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 2017-06-19 00:45:23 PDT
[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 ()
Comment 1 Fujii Hironori 2017-07-18 21:22:35 PDT
*** Bug 173957 has been marked as a duplicate of this bug. ***
Comment 2 Fujii Hironori 2017-11-07 00:32:03 PST
In AXObjectCache::performDeferredCacheUpdate:

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

element was already destructed.
Comment 3 Fujii Hironori 2017-11-07 00:36:09 PST
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
Comment 4 Radar WebKit Bug Importer 2017-11-07 06:22:39 PST
<rdar://problem/35386393>
Comment 5 Fujii Hironori 2017-11-08 03:02:07 PST
Created attachment 326319 [details]
WIP patch
Comment 6 Fujii Hironori 2017-11-08 18:15:43 PST
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.
Comment 7 Fujii Hironori 2017-11-08 21:52:07 PST
Created attachment 326429 [details]
WIP patch
Comment 8 Fujii Hironori 2017-12-04 01:50:08 PST
Created attachment 328334 [details]
Patch
Comment 9 Michael Catanzaro 2017-12-04 07:08:54 PST
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
Comment 10 Michael Catanzaro 2017-12-04 07:10:36 PST
Chris F., do you want to review this one?
Comment 11 zalan 2017-12-04 07:31:18 PST
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 12 chris fleizach 2017-12-04 08:52:39 PST
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
Comment 13 zalan 2017-12-04 08:59:27 PST
(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?
Comment 14 zalan 2017-12-04 09:00:19 PST
(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.
Comment 15 Michael Catanzaro 2017-12-04 12:28:10 PST
I'm adding a crash expectation for this test, so please remove the expectation when landing.
Comment 16 Fujii Hironori 2017-12-04 17:39:40 PST
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
Comment 17 Fujii Hironori 2017-12-04 17:39:51 PST
Anyway, I don't understand AX at all. Feel free to take over this bug. Thanks in advance.
Comment 18 zalan 2017-12-06 16:58:34 PST
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!
Comment 19 Fujii Hironori 2017-12-06 19:01:21 PST
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.
Comment 20 zalan 2017-12-06 19:03:53 PST
(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 21 WebKit Commit Bot 2017-12-06 19:23:55 PST
Comment on attachment 328670 [details]
Unreviewed gardening patch

Clearing flags on attachment: 328670

Committed r225614: <https://trac.webkit.org/changeset/225614>
Comment 22 WebKit Commit Bot 2017-12-06 19:23:57 PST
All reviewed patches have been landed.  Closing bug.