Many TSAN errors in fontconfig when switching to in-tree fontconfig build |
||
Issue descriptionPlease see this log: https://ci.chromium.org/buildbot/tryserver.chromium.linux/linux_chromium_tsan_rel_ng/216630 There are many new failures in content_browsertests caused by instrumenting fontconfig with TSAN. These failures were not seen before because we used to use the (uninstrumented) system library. For now, the failures will be suppressed. The issue appears to be that the main thread and the sandbox IPC broker both make fontconfig calls. Fontconfig is supposed to be thread-safe, so this is either a bug in fontconfig or false-positives in TSAN.
,
Dec 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4f1a60552748d150783655ee6f7b6f7084e3af95 commit 4f1a60552748d150783655ee6f7b6f7084e3af95 Author: Tom Anderson <thomasanderson@chromium.org> Date: Fri Dec 15 15:53:31 2017 Statically link fontconfig on Linux This CL switches Linux to use in-tree fontconfig builds. It exposes some leaks and races detected by LSAN and TSAN. For now, these will be suppressed since these issues were preexisting, but instrumenting fontconfig exposed the issues. R=dnicoara@chromium.org,dpranke@chromium.org BUG=795110, 795148 Change-Id: Ia75db4ced6ec78a5f0610af9ebc78a87840b86f7 Reviewed-on: https://chromium-review.googlesource.com/826403 Commit-Queue: Daniel Nicoara <dnicoara@chromium.org> Reviewed-by: Daniel Nicoara <dnicoara@chromium.org> Reviewed-by: Dirk Pranke <dpranke@chromium.org> Cr-Commit-Position: refs/heads/master@{#524389} [add] https://crrev.com/4f1a60552748d150783655ee6f7b6f7084e3af95/build/linux/unbundle/fontconfig.gn [modify] https://crrev.com/4f1a60552748d150783655ee6f7b6f7084e3af95/build/linux/unbundle/replace_gn_files.py [modify] https://crrev.com/4f1a60552748d150783655ee6f7b6f7084e3af95/build/sanitizers/lsan_suppressions.cc [modify] https://crrev.com/4f1a60552748d150783655ee6f7b6f7084e3af95/build/sanitizers/tsan_suppressions.cc [modify] https://crrev.com/4f1a60552748d150783655ee6f7b6f7084e3af95/chrome/installer/linux/debian/dist_package_versions.json [modify] https://crrev.com/4f1a60552748d150783655ee6f7b6f7084e3af95/chrome/installer/linux/debian/update_dist_package_versions.py [modify] https://crrev.com/4f1a60552748d150783655ee6f7b6f7084e3af95/chrome/installer/linux/rpm/dist_package_provides.json [modify] https://crrev.com/4f1a60552748d150783655ee6f7b6f7084e3af95/chrome/installer/linux/rpm/update_package_provides.py [modify] https://crrev.com/4f1a60552748d150783655ee6f7b6f7084e3af95/third_party/WebKit/LayoutTests/platform/linux/fast/text/unicode-fallback-font-expected.png [modify] https://crrev.com/4f1a60552748d150783655ee6f7b6f7084e3af95/third_party/fontconfig/BUILD.gn [modify] https://crrev.com/4f1a60552748d150783655ee6f7b6f7084e3af95/third_party/fontconfig/fontconfig.gni
,
Dec 15 2017
Relevant comment from evanm: " FYI, derat randomly was talking to me about font code and I happened to notice you changed this code to use a thread. The reason this was a process initially is because this code backs into fontconfig and fontconfig has (had?) random statics, so it wasn't thread-safe. It's not sufficient to stick just some locks in this code because you don't know who else is calling into fontconfig -- in the GTK days, the UI thread would randomly do so, but even now it's plausible that UI code (backed by Skia) is still backing onto fontconfig. And in any case even with a lock (or by proxying all calls over to the UI thread, which is the way to be extra sure about concurrent access without using any locks) it would mean that the the UI could block when a renderer wants random fonts, which seems like sadness. It may be the case that fontconfig is now thread safe. I notice their changelog from Fontconfig 2.10.91 mentions making it thread safe. That version appears in Ubuntu Trusty (which is was made available within corp Google in the last few months, but definitely not back in the days we were writing all this code). http://www.freedesktop.org/wiki/Software/fontconfig/Devel/ I'll still note that if you (a) use locks around every place you touch fontconfig and (b) use it from multiple threads, you still run into the problem that if the UI happens to run into a weird glyph that causes it to back into fontconfig, then a page that also has a bunch of weird glyphs could plausibly cause UI hangs while it's holding a fontconfig lock while ferreting around on the disk for a font file. (But these days maybe Chrome's UI has enough random hangs that I don't think adding one more would be so awful, and the chance is pretty remote. I expect equivalents of this hypothetical hang also occur in Windows Chrome with fonts.) In all, in normal Chrome code when the renderer wants something it's supposed to access the browser via the normal IPC system. All this SandboxIPCProcess junk is just a workaround for this fontconfig mess, but if you really think it's safe to run it on a thread then you should delete all this and make these font calls work over the standard browser IPC system. PS: any time anyone asked me what ChromeOS should do, I recommended they remove fontconfig, since it seems like ChromeOS's font needs are pretty simple. I suspect more time has been spent making it work (scripts to regenerate cache files etc.) than it would to replace it, but I never had a lot of visibility into that. PPS: it's been a long time since I looked at Chrome code, sorry for all the hacks! " From https://bugs.chromium.org/p/chromium/issues/detail?id=322185#c95
,
Dec 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5a8428d621c9e9c8f5bcfa9ca97ae66002207d8c commit 5a8428d621c9e9c8f5bcfa9ca97ae66002207d8c Author: Lukasz Anforowicz <lukasza@chromium.org> Date: Tue Dec 19 23:05:34 2017 Revert "Statically link fontconfig on Linux" This reverts commit 4f1a60552748d150783655ee6f7b6f7084e3af95. Reason for revert: This CL caused time outs in browser tests - see https://crbug.com/796292 Original change's description: > Statically link fontconfig on Linux > > This CL switches Linux to use in-tree fontconfig builds. It exposes some leaks > and races detected by LSAN and TSAN. For now, these will be suppressed since > these issues were preexisting, but instrumenting fontconfig exposed the issues. > > R=dnicoara@chromium.org,dpranke@chromium.org > BUG=795110, 795148 > > Change-Id: Ia75db4ced6ec78a5f0610af9ebc78a87840b86f7 > Reviewed-on: https://chromium-review.googlesource.com/826403 > Commit-Queue: Daniel Nicoara <dnicoara@chromium.org> > Reviewed-by: Daniel Nicoara <dnicoara@chromium.org> > Reviewed-by: Dirk Pranke <dpranke@chromium.org> > Cr-Commit-Position: refs/heads/master@{#524389} TBR=dpranke@chromium.org,dnicoara@chromium.org,thomasanderson@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 795110, 795148 Change-Id: Id224fdcbfb0ca3373f6219b66252a8970072675b Reviewed-on: https://chromium-review.googlesource.com/834869 Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org> Reviewed-by: Dirk Pranke <dpranke@chromium.org> Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org> Cr-Commit-Position: refs/heads/master@{#525173} [delete] https://crrev.com/7f446eb0ecff4014853501cd6d50fcf8ff6fa98b/build/linux/unbundle/fontconfig.gn [modify] https://crrev.com/5a8428d621c9e9c8f5bcfa9ca97ae66002207d8c/build/linux/unbundle/replace_gn_files.py [modify] https://crrev.com/5a8428d621c9e9c8f5bcfa9ca97ae66002207d8c/build/sanitizers/lsan_suppressions.cc [modify] https://crrev.com/5a8428d621c9e9c8f5bcfa9ca97ae66002207d8c/build/sanitizers/tsan_suppressions.cc [modify] https://crrev.com/5a8428d621c9e9c8f5bcfa9ca97ae66002207d8c/chrome/installer/linux/debian/dist_package_versions.json [modify] https://crrev.com/5a8428d621c9e9c8f5bcfa9ca97ae66002207d8c/chrome/installer/linux/debian/update_dist_package_versions.py [modify] https://crrev.com/5a8428d621c9e9c8f5bcfa9ca97ae66002207d8c/chrome/installer/linux/rpm/dist_package_provides.json [modify] https://crrev.com/5a8428d621c9e9c8f5bcfa9ca97ae66002207d8c/chrome/installer/linux/rpm/update_package_provides.py [modify] https://crrev.com/5a8428d621c9e9c8f5bcfa9ca97ae66002207d8c/third_party/WebKit/LayoutTests/platform/linux/fast/text/unicode-fallback-font-expected.png [modify] https://crrev.com/5a8428d621c9e9c8f5bcfa9ca97ae66002207d8c/third_party/fontconfig/BUILD.gn [modify] https://crrev.com/5a8428d621c9e9c8f5bcfa9ca97ae66002207d8c/third_party/fontconfig/fontconfig.gni
,
Dec 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/65f919d2d86e54c851b0327740a330d152b07ed5 commit 65f919d2d86e54c851b0327740a330d152b07ed5 Author: Tom Anderson <thomasanderson@chromium.org> Date: Fri Dec 22 06:20:09 2017 Reland "Statically link fontconfig on Linux" This reverts commit 5a8428d621c9e9c8f5bcfa9ca97ae66002207d8c Reason for revert: See comment https://bugs.chromium.org/p/chromium/issues/detail?id=796292#c28 > This reverts commit 4f1a60552748d150783655ee6f7b6f7084e3af95. > > Reason for revert: This CL caused time outs in browser tests - see https://crbug.com/796292 > > Original change's description: > > Statically link fontconfig on Linux > > > > This CL switches Linux to use in-tree fontconfig builds. It exposes some leaks > > and races detected by LSAN and TSAN. For now, these will be suppressed since > > these issues were preexisting, but instrumenting fontconfig exposed the issues. > > > > R=dnicoara@chromium.org,dpranke@chromium.org > > BUG=795110, 795148 > > > > Change-Id: Ia75db4ced6ec78a5f0610af9ebc78a87840b86f7 > > Reviewed-on: https://chromium-review.googlesource.com/826403 > > Commit-Queue: Daniel Nicoara <dnicoara@chromium.org> > > Reviewed-by: Daniel Nicoara <dnicoara@chromium.org> > > Reviewed-by: Dirk Pranke <dpranke@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#524389} > > TBR=dpranke@chromium.org,dnicoara@chromium.org,thomasanderson@chromium.org > > # Not skipping CQ checks because original CL landed > 1 day ago. > > Bug: 795110, 795148 > Change-Id: Id224fdcbfb0ca3373f6219b66252a8970072675b > Reviewed-on: https://chromium-review.googlesource.com/834869 > Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> > Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org> > Reviewed-by: Dirk Pranke <dpranke@chromium.org> > Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org> > Cr-Commit-Position: refs/heads/master@{#525173} R=dpranke@chromium.org TBR=dnicoara@chromium.org BUG=795110, 795148 , 796292 Change-Id: I47a2225cb9cc255721b521cbb790d9eb70e0b3b9 Reviewed-on: https://chromium-review.googlesource.com/841627 Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> Reviewed-by: Dirk Pranke <dpranke@chromium.org> Commit-Queue: Thomas Anderson <thomasanderson@chromium.org> Cr-Commit-Position: refs/heads/master@{#525945} [add] https://crrev.com/65f919d2d86e54c851b0327740a330d152b07ed5/build/linux/unbundle/fontconfig.gn [modify] https://crrev.com/65f919d2d86e54c851b0327740a330d152b07ed5/build/linux/unbundle/replace_gn_files.py [modify] https://crrev.com/65f919d2d86e54c851b0327740a330d152b07ed5/build/sanitizers/lsan_suppressions.cc [modify] https://crrev.com/65f919d2d86e54c851b0327740a330d152b07ed5/build/sanitizers/tsan_suppressions.cc [modify] https://crrev.com/65f919d2d86e54c851b0327740a330d152b07ed5/chrome/installer/linux/debian/dist_package_versions.json [modify] https://crrev.com/65f919d2d86e54c851b0327740a330d152b07ed5/chrome/installer/linux/debian/update_dist_package_versions.py [modify] https://crrev.com/65f919d2d86e54c851b0327740a330d152b07ed5/chrome/installer/linux/rpm/dist_package_provides.json [modify] https://crrev.com/65f919d2d86e54c851b0327740a330d152b07ed5/chrome/installer/linux/rpm/update_package_provides.py [modify] https://crrev.com/65f919d2d86e54c851b0327740a330d152b07ed5/third_party/WebKit/LayoutTests/platform/linux/fast/text/unicode-fallback-font-expected.png [modify] https://crrev.com/65f919d2d86e54c851b0327740a330d152b07ed5/third_party/fontconfig/BUILD.gn [modify] https://crrev.com/65f919d2d86e54c851b0327740a330d152b07ed5/third_party/fontconfig/fontconfig.gni
,
Jan 8
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot |
||
►
Sign in to add a comment |
||
Comment 1 by thomasanderson@chromium.org
, Dec 15 2017