FontCache::CrashWithFontInfo does not record enough font info when crashing |
|||
Issue description
Chrome Version: 62.0.3188.0
OS: All
I noticed this problem through a mixture of crash-dump analysis and code inspection. The problematic function is here:
void FontCache::CrashWithFontInfo(const FontDescription* font_description) {
FontCache* font_cache = FontCache::GetFontCache();
SkFontMgr* font_mgr = nullptr;
int num_families = std::numeric_limits<int>::min();
if (font_cache) {
font_mgr = font_cache->font_manager_.get();
if (font_mgr)
num_families = font_mgr->countFamilies();
}
FontDescription font_description_copy = *font_description;
WTF::debug::Alias(&font_description_copy);
WTF::debug::Alias(&font_cache);
WTF::debug::Alias(&font_mgr);
WTF::debug::Alias(&num_families);
CHECK(false);
}
The WTF::debug::Alias calls force the compiler to retain the four local variables. However two of these local variables are pointers and retaining them is not actually particularly useful, since what we care about is the data that they point to, and there is no guarantee that that information will be retained. In practice it is not.
If the FontCache and SkFontMgr information is important then it should be copied into locals, either copying individual fields or entire objects. Or, perhaps the Alias calls with pointer members should be removed to avoid raising expectations.
See crash ID e6db581c2d4f7675 for one example.
,
Aug 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/263423ff606d241556e0cadd05c2bfd21f0fbebb commit 263423ff606d241556e0cadd05c2bfd21f0fbebb Author: Bruce Dawson <brucedawson@chromium.org> Date: Fri Aug 18 03:32:26 2017 Document effective use of base::debug::Alias Many uses of Alias(&var) end up being ineffective because var is a pointer and the crash dump mostly only records the contents of the thread stacks - therefore we end up with a pointer but not the memory that it points to. This change documents this issue and gives examples of effective use of base::debug::Alias. BUG= 756544 , 756589 Change-Id: I6095d5018e72c159bbe907c6294d7a48cf28fd82 Reviewed-on: https://chromium-review.googlesource.com/619112 Reviewed-by: Nico Weber <thakis@chromium.org> Commit-Queue: Bruce Dawson <brucedawson@chromium.org> Cr-Commit-Position: refs/heads/master@{#495449} [modify] https://crrev.com/263423ff606d241556e0cadd05c2bfd21f0fbebb/base/debug/alias.h [modify] https://crrev.com/263423ff606d241556e0cadd05c2bfd21f0fbebb/third_party/WebKit/Source/platform/wtf/debug/Alias.h
,
Aug 21 2017
Thanks, Bruce, for the details and adding the documentation to use Alias effectively. Koji, would we still profit from the additional crash debug info? IIRC you added this when we were trying to figure out the cause for the unreproduced Windows empty font handle crashes? Compare https://bugs.chromium.org/p/chromium/issues/detail?id=561873#c168
,
Aug 22 2017
Yeah, this was really helpful to understand Alias, thank you Bruce. While two of the four weren't useful due to incorrect usage, one of the four, |num_families| being 0 gave me a hint for the next step. I'm currently thinking RPC is more likely the cause, but unable to take the next step yet. drott@, if you need other info in FontCache/SkFontMgr to come up with the other possible ideas, let's add them as local copies, or we can remove the two as Bruce suggested. WDYT?
,
Nov 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2ce53addebe44e8b97f6b077792023de40c9fdb4 commit 2ce53addebe44e8b97f6b077792023de40c9fdb4 Author: Bruce Dawson <brucedawson@chromium.org> Date: Wed Nov 29 21:19:09 2017 Remove ineffective use of base::debug::Alias from FontCache Passing the address of pointer variables to Alias has very minimal value because it causes the pointer to be retained but not what it points at. These ineffective uses should be removed to avoid confusion or incorrect expectations. Bug: 756544 , 756589 Change-Id: I77d91a3db5cb41d972d0330ce7034be9bee34a37 Reviewed-on: https://chromium-review.googlesource.com/793896 Reviewed-by: Dominik Röttsches <drott@chromium.org> Reviewed-by: Koji Ishii <kojii@chromium.org> Commit-Queue: Bruce Dawson <brucedawson@chromium.org> Cr-Commit-Position: refs/heads/master@{#520246} [modify] https://crrev.com/2ce53addebe44e8b97f6b077792023de40c9fdb4/third_party/WebKit/Source/platform/fonts/FontCache.cpp
,
Feb 26 2018
|
|||
►
Sign in to add a comment |
|||
Comment 1 by brucedaw...@chromium.org
, Aug 17 2017