| Summary: | [GTK] Crash destroying WebCore::FileMonitor | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||
| Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | bfulgham, bugs-noreply, cgarcia, magomez, mcatanzaro, wilander | ||||
| Priority: | P2 | ||||||
| Version: | Other | ||||||
| Hardware: | PC | ||||||
| OS: | Linux | ||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=179909 https://bugs.webkit.org/show_bug.cgi?id=181231 |
||||||
| Attachments: |
|
||||||
|
Description
Michael Catanzaro
2017-12-22 14:22:41 PST
*** Bug 180806 has been marked as a duplicate of this bug. *** Also: fast/html/menuitem-element.html 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. 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. Created attachment 330311 [details]
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.
(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. I'll review the patch in the afternoon. (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. (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? (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. Comment on attachment 330311 [details] 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. Please also remove all the crash expectations with this patch. Otherwise we will not notice whether it is still crashing or not. (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 Committed r226355: <https://trac.webkit.org/changeset/226355> |