181138 2017-12-22 14:22:41 -0800 [GTK] Crash destroying WebCore::FileMonitor 2018-01-03 00:23:40 -0800 1 1 1 Unclassified WebKit WebKitGTK Other PC Linux RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=179909 https://bugs.webkit.org/show_bug.cgi?id=181231 P2 Normal --- 1 mcatanzaro webkit-unassigned bfulgham bugs-noreply cgarcia magomez mcatanzaro wilander oldest_to_newest 1384556 0 mcatanzaro 2017-12-22 14:22:41 -0800 I thought we had fixed this in bug #179909, but apparently something is still wrong. Layout tests fast/scrolling/rtl-scrollbars-animation-property.html and intersection-observer/intersection-observer-entry-interface.html are both flaky crashes: Thread 1 (Thread 0x7ff833fff700 (LWP 7884)): #0 0x00007ff89a537b75 in g_type_check_instance_is_fundamentally_a () at /home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/gobject/gtype.c:4023 #1 0x00007ff89a5182b5 in g_object_unref () at /home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/gobject/gobject.c:3227 #2 0x00007ff89d74ca97 in _ZN3WTF8FunctionIFvvEE15CallableWrapperIZN7WebCore11FileMonitorC4ERKNS_6StringEONS_3RefINS_9WorkQueueENS_13DumbPtrTraitsISA_EEEEONS0_IFvNS5_14FileChangeTypeEEEEEUlvE_E4callEv () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #3 0x00007ff89e980e57 in _ZN3WTF7RunLoop11performWorkEv () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #4 0x00007ff89e9be4c9 in _ZZN3WTF7RunLoopC4EvENUlPvE_4_FUNES1_ () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #5 0x00007ff89a43781a in g_main_dispatch () at /home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3148 #6 g_main_context_dispatch () at /home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3813 #7 0x00007ff89a437ba8 in g_main_context_iterate () at /home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3886 #8 0x00007ff89a437ec2 in g_main_loop_run () at /home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:4082 #9 0x00007ff89e9bee90 in _ZN3WTF7RunLoop3runEv () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #10 0x00007ff89e9825ab in _ZN3WTF6Thread10entryPointEPNS0_16NewThreadContextE () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #11 0x00007ff89e9bd249 in _ZN3WTFL19wtfThreadEntryPointEPv () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #12 0x00007ff8977ba494 in start_thread (arg=0x7ff833fff700) at pthread_create.c:333 #13 0x00007ff893f9d93f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:97 Updating expectations accordingly. fast/scrolling/rtl-scrollbars-animation-property.html has been crashing for a while, first one I see is r223831. But, interestingly, the first crash I see for intersection-observer/intersection-observer-entry-interface.html is at r225145, so this one didn't start crashing until after the change in bug #179909 landed. 1384733 1 mcatanzaro 2017-12-26 10:19:44 -0800 *** Bug 180806 has been marked as a duplicate of this bug. *** 1384735 2 mcatanzaro 2017-12-26 10:21:04 -0800 Also: fast/html/menuitem-element.html 1385096 3 mcatanzaro 2018-01-01 20:15:54 -0800 Also: fast/attachment/attachment-action.html fast/canvas/2d.currentPoint.html fast/text/international/system-language/arabic-glyph-cache-fill-combine.html I expect this must affect a huge number of tests. 1385103 4 cgarcia 2018-01-02 02:22:44 -0800 I can't reproduce this, but I think we can avoid all race conditions by always creating and destroying the platform file monitor in the work queue thread synchronously. However, I wonder if we should revert the patch that added support for resource load stats in the glib based ports. Does it make sense to have it without the machine learning part? The layout tests were passing when we added the support, but that's no longer the case, all of them are timing out now and John skipped them in r223253. We should fix FileMonitor in any case, because even if it's only used by resource load stats right now, it's a generic implementation that could be used by other parts eventually. 1385104 5 330311 cgarcia 2018-01-02 02:28:02 -0800 Created attachment 330311 Speculative fix Since resource load stats tests are skipped, we should disable the feature in WTR. That would fix these crashes for sure, but let's do it a few days after this patch lands to ensure it fixes the crashes. 1385130 6 mcatanzaro 2018-01-02 08:04:39 -0800 (In reply to Carlos Garcia Campos from comment #4) > However, I wonder if we should revert the patch that added support for > resource load stats in the glib based ports. Does it make sense to have it > without the machine learning part? The layout tests were passing when we > added the support, but that's no longer the case, all of them are timing out > now and John skipped them in r223253. This has been bothering me for a while. We should revert it, yes, if we don't have time to fix it. I don't know how essential the machine learning portion is. We should ask the Apple developers who designed it for advice. We can probably implement it ourselves with sufficient time; problem is, machine learning is tricky and requires a lot more experience with math than I have myself, so we'll surely need guidance. > We should fix FileMonitor in any case, because even if it's only used by > resource load stats right now, it's a generic implementation that could be > used by other parts eventually. Yes. 1385131 7 mcatanzaro 2018-01-02 08:05:42 -0800 I'll review the patch in the afternoon. 1385133 8 bfulgham 2018-01-02 08:29:38 -0800 (In reply to Carlos Garcia Campos from comment #4) > However, I wonder if we should revert the patch that added support for > resource load stats in the glib based ports. Does it make sense to have it > without the machine learning part You should be able to use the threshold-based classifier without needing to use the ML based engine. It works pretty well (or at least it did back when we were using it). I think you get that version automatically when you build without the libsvm features enabled. 1385136 9 cgarcia 2018-01-02 08:34:32 -0800 (In reply to Brent Fulgham from comment #8) > (In reply to Carlos Garcia Campos from comment #4) > > However, I wonder if we should revert the patch that added support for > > resource load stats in the glib based ports. Does it make sense to have it > > without the machine learning part > > You should be able to use the threshold-based classifier without needing to > use the ML based engine. It works pretty well (or at least it did back when > we were using it). I think you get that version automatically when you build > without the libsvm features enabled. Thanks Brent, I guess that's what we had before it broke in r223253. How can we test it apart from running the layout tests? 1385140 10 mcatanzaro 2018-01-02 08:44:26 -0800 (In reply to Carlos Garcia Campos from comment #9) > Thanks Brent, I guess that's what we had before it broke in r223253. How can > we test it apart from running the layout tests? From a quick skim of that patch, it looks like we just have to implement TestController::isStatisticsRegisteredAsSubFrameUnder and TestController::isStatisticsRegisteredAsRedirectingTo to get the tests working again. 1385276 11 330311 mcatanzaro 2018-01-02 15:31:18 -0800 Comment on attachment 330311 Speculative fix View in context: https://bugs.webkit.org/attachment.cgi?id=330311&action=review I think the code changes are right. It would be good to add a comment to document the thread-safety semantics of FileMonitor, since this has proven to be a tricky bit of code. Right now, it's not clear to the reader why the create/destroy operations need to be synchronous. > Source/WebCore/platform/glib/FileMonitorGLib.cpp:60 > + // The monitor can be destroyed in the work queue thread. > + if (&m_handlerQueue->runLoop() == &RunLoop::current()) > + return; Good catch. 1385277 12 mcatanzaro 2018-01-02 15:35:55 -0800 Please also remove all the crash expectations with this patch. Otherwise we will not notice whether it is still crashing or not. 1385347 13 mcatanzaro 2018-01-02 17:53:01 -0800 (In reply to Michael Catanzaro from comment #10) > From a quick skim of that patch, it looks like we just have to implement > TestController::isStatisticsRegisteredAsSubFrameUnder and > TestController::isStatisticsRegisteredAsRedirectingTo to get the tests > working again. Bug #181231 1385390 14 cgarcia 2018-01-03 00:23:40 -0800 Committed r226355: <https://trac.webkit.org/changeset/226355> 330311 2018-01-02 02:28:02 -0800 2018-01-02 15:31:18 -0800 Speculative fix wcore-file-monitor-crash.diff text/plain 4000 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCAxOWY5NTM2OTVmMS4uNWMxZDc4NmZjMDYgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9XZWJDb3JlL0NoYW5nZUxvZworKysgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKQEAgLTEg KzEsMTYgQEAKKzIwMTgtMDEtMDIgIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2Fs aWEuY29tPgorCisgICAgICAgIFtHVEtdIENyYXNoIGRlc3Ryb3lpbmcgV2ViQ29yZTo6RmlsZU1v bml0b3IKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE4 MTEzOAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEVu c3VyZSB0aGF0IHBsYXRmb3JtIGZpbGUgbW9uaXRvciBpcyBhbHdheXMgY3JlYXRlZCBhbmQgZGVz dHJveWVkIGluIHRoZSB3b3JrIHF1ZXVlIHRocmVhZCBzeW5jaHJvbm91c2x5LgorCisgICAgICAg ICogcGxhdGZvcm0vRmlsZU1vbml0b3IuaDoKKyAgICAgICAgKiBwbGF0Zm9ybS9nbGliL0ZpbGVN b25pdG9yR0xpYi5jcHA6CisgICAgICAgIChXZWJDb3JlOjpGaWxlTW9uaXRvcjo6RmlsZU1vbml0 b3IpOgorICAgICAgICAoV2ViQ29yZTo6RmlsZU1vbml0b3I6On5GaWxlTW9uaXRvcik6CisgICAg ICAgIChXZWJDb3JlOjpGaWxlTW9uaXRvcjo6ZGlkQ2hhbmdlKToKKwogPT0gUm9sbGVkIG92ZXIg dG8gQ2hhbmdlTG9nLTIwMTgtMDEtMDEgPT0KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3Bs YXRmb3JtL0ZpbGVNb25pdG9yLmggYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9GaWxlTW9uaXRv ci5oCmluZGV4IDIyNWMzMzU3YWQwLi5mOTQxMmU1MzJkNyAxMDA2NDQKLS0tIGEvU291cmNlL1dl YkNvcmUvcGxhdGZvcm0vRmlsZU1vbml0b3IuaAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9y bS9GaWxlTW9uaXRvci5oCkBAIC02MSw3ICs2MSw2IEBAIHByaXZhdGU6CiAgICAgUmVmPFdvcmtR dWV1ZT4gbV9oYW5kbGVyUXVldWU7CiAgICAgRnVuY3Rpb248dm9pZChGaWxlQ2hhbmdlVHlwZSk+ IG1fbW9kaWZpY2F0aW9uSGFuZGxlcjsKICAgICBHUmVmUHRyPEdGaWxlTW9uaXRvcj4gbV9wbGF0 Zm9ybU1vbml0b3I7Ci0gICAgR1JlZlB0cjxHQ2FuY2VsbGFibGU+IG1fY2FuY2VsbGFibGU7CiAj ZW5kaWYKIH07CiAKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dsaWIvRmls ZU1vbml0b3JHTGliLmNwcCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dsaWIvRmlsZU1vbml0 b3JHTGliLmNwcAppbmRleCA4NGQ0OTZhNzQ4My4uNzQxMTZkOWYxODMgMTAwNjQ0Ci0tLSBhL1Nv dXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dsaWIvRmlsZU1vbml0b3JHTGliLmNwcAorKysgYi9Tb3Vy Y2UvV2ViQ29yZS9wbGF0Zm9ybS9nbGliL0ZpbGVNb25pdG9yR0xpYi5jcHAKQEAgLTI4LDYgKzI4 LDcgQEAKIAogI2luY2x1ZGUgIkZpbGVTeXN0ZW0uaCIKICNpbmNsdWRlIDx3dGYvZ2xpYi9HVW5p cXVlUHRyLmg+CisjaW5jbHVkZSA8d3RmL3RocmVhZHMvQmluYXJ5U2VtYXBob3JlLmg+CiAKIG5h bWVzcGFjZSBXZWJDb3JlIHsKIApAQCAtMzgsMjQgKzM5LDMyIEBAIEZpbGVNb25pdG9yOjpGaWxl TW9uaXRvcihjb25zdCBTdHJpbmcmIHBhdGgsIFJlZjxXb3JrUXVldWU+JiYgaGFuZGxlclF1ZXVl LCBXVEY6CiAgICAgaWYgKHBhdGguaXNFbXB0eSgpIHx8ICFtX21vZGlmaWNhdGlvbkhhbmRsZXIp CiAgICAgICAgIHJldHVybjsKIAotICAgIG1fY2FuY2VsbGFibGUgPSBhZG9wdEdSZWYoZ19jYW5j ZWxsYWJsZV9uZXcoKSk7Ci0gICAgbV9oYW5kbGVyUXVldWUtPmRpc3BhdGNoKFt0aGlzLCBjYW5j ZWxsYWJsZSA9IG1fY2FuY2VsbGFibGUsIHBhdGggPSBwYXRoLmlzb2xhdGVkQ29weSgpXSB7Ci0g ICAgICAgIGlmIChnX2NhbmNlbGxhYmxlX2lzX2NhbmNlbGxlZChjYW5jZWxsYWJsZS5nZXQoKSkp Ci0gICAgICAgICAgICByZXR1cm47CisgICAgQmluYXJ5U2VtYXBob3JlIHNlbWFwaG9yZTsKKyAg ICBtX2hhbmRsZXJRdWV1ZS0+ZGlzcGF0Y2goWyZdIHsKICAgICAgICAgYXV0byBmaWxlID0gYWRv cHRHUmVmKGdfZmlsZV9uZXdfZm9yX3BhdGgoRmlsZVN5c3RlbTo6ZmlsZVN5c3RlbVJlcHJlc2Vu dGF0aW9uKHBhdGgpLmRhdGEoKSkpOwogICAgICAgICBHVW5pcXVlT3V0UHRyPEdFcnJvcj4gZXJy b3I7Ci0gICAgICAgIG1fcGxhdGZvcm1Nb25pdG9yID0gYWRvcHRHUmVmKGdfZmlsZV9tb25pdG9y KGZpbGUuZ2V0KCksIEdfRklMRV9NT05JVE9SX05PTkUsIG1fY2FuY2VsbGFibGUuZ2V0KCksICZl cnJvci5vdXRQdHIoKSkpOwotICAgICAgICBpZiAoIW1fcGxhdGZvcm1Nb25pdG9yKSB7CisgICAg ICAgIG1fcGxhdGZvcm1Nb25pdG9yID0gYWRvcHRHUmVmKGdfZmlsZV9tb25pdG9yKGZpbGUuZ2V0 KCksIEdfRklMRV9NT05JVE9SX05PTkUsIG51bGxwdHIsICZlcnJvci5vdXRQdHIoKSkpOworICAg ICAgICBpZiAobV9wbGF0Zm9ybU1vbml0b3IpCisgICAgICAgICAgICBnX3NpZ25hbF9jb25uZWN0 KG1fcGxhdGZvcm1Nb25pdG9yLmdldCgpLCAiY2hhbmdlZCIsIEdfQ0FMTEJBQ0soZmlsZUNoYW5n ZWRDYWxsYmFjayksIHRoaXMpOworICAgICAgICBlbHNlCiAgICAgICAgICAgICBXVEZMb2dBbHdh eXMoIkZhaWxlZCB0byBjcmVhdGUgYSBtb25pdG9yIGZvciBwYXRoICVzOiAlcyIsIHBhdGgudXRm OCgpLmRhdGEoKSwgZXJyb3ItPm1lc3NhZ2UpOwotICAgICAgICAgICAgcmV0dXJuOwotICAgICAg ICB9Ci0gICAgICAgIGdfc2lnbmFsX2Nvbm5lY3QobV9wbGF0Zm9ybU1vbml0b3IuZ2V0KCksICJj aGFuZ2VkIiwgR19DQUxMQkFDSyhmaWxlQ2hhbmdlZENhbGxiYWNrKSwgdGhpcyk7CisgICAgICAg IHNlbWFwaG9yZS5zaWduYWwoKTsKICAgICB9KTsKKyAgICBzZW1hcGhvcmUud2FpdChXYWxsVGlt ZTo6aW5maW5pdHkoKSk7CiB9CiAKIEZpbGVNb25pdG9yOjp+RmlsZU1vbml0b3IoKQogewotICAg IGdfY2FuY2VsbGFibGVfY2FuY2VsKG1fY2FuY2VsbGFibGUuZ2V0KCkpOworICAgIC8vIFRoZSBt b25pdG9yIGNhbiBiZSBkZXN0cm95ZWQgaW4gdGhlIHdvcmsgcXVldWUgdGhyZWFkLgorICAgIGlm ICgmbV9oYW5kbGVyUXVldWUtPnJ1bkxvb3AoKSA9PSAmUnVuTG9vcDo6Y3VycmVudCgpKQorICAg ICAgICByZXR1cm47CisKKyAgICBCaW5hcnlTZW1hcGhvcmUgc2VtYXBob3JlOworICAgIG1faGFu ZGxlclF1ZXVlLT5kaXNwYXRjaChbJl0geworICAgICAgICBtX3BsYXRmb3JtTW9uaXRvciA9IG51 bGxwdHI7CisgICAgICAgIHNlbWFwaG9yZS5zaWduYWwoKTsKKyAgICB9KTsKKyAgICBzZW1hcGhv cmUud2FpdChXYWxsVGltZTo6aW5maW5pdHkoKSk7CiB9CiAKIHZvaWQgRmlsZU1vbml0b3I6OmZp bGVDaGFuZ2VkQ2FsbGJhY2soR0ZpbGVNb25pdG9yKiwgR0ZpbGUqLCBHRmlsZSosIEdGaWxlTW9u aXRvckV2ZW50IGV2ZW50LCBGaWxlTW9uaXRvciogbW9uaXRvcikKQEAgLTc2LDE0ICs4NSw5IEBA IHZvaWQgRmlsZU1vbml0b3I6OmZpbGVDaGFuZ2VkQ2FsbGJhY2soR0ZpbGVNb25pdG9yKiwgR0Zp bGUqLCBHRmlsZSosIEdGaWxlTW9uaXRvCiB2b2lkIEZpbGVNb25pdG9yOjpkaWRDaGFuZ2UoRmls ZUNoYW5nZVR5cGUgdHlwZSkKIHsKICAgICBBU1NFUlQoIWlzTWFpblRocmVhZCgpKTsKLSAgICBp ZiAoZ19jYW5jZWxsYWJsZV9pc19jYW5jZWxsZWQobV9jYW5jZWxsYWJsZS5nZXQoKSkpIHsKLSAg ICAgICAgbV9wbGF0Zm9ybU1vbml0b3IgPSBudWxscHRyOwotICAgICAgICByZXR1cm47Ci0gICAg fQotCi0gICAgbV9tb2RpZmljYXRpb25IYW5kbGVyKHR5cGUpOwogICAgIGlmICh0eXBlID09IEZp bGVDaGFuZ2VUeXBlOjpSZW1vdmFsKQogICAgICAgICBtX3BsYXRmb3JtTW9uaXRvciA9IG51bGxw dHI7CisgICAgbV9tb2RpZmljYXRpb25IYW5kbGVyKHR5cGUpOwogfQogCiB9IC8vIG5hbWVzcGFj ZSBXZWJDb3JlCg==