Segfault in extensions::Extension::GetManifestData |
|||||||
Issue descriptionObservation: crash/51105c7878000000 CHROMEOS_RELEASE_VERSION: 9592.42.0 CHROME VERSION: 60.0.3112.52 beta There's another bug for the leaf function open (crbug.com/533262), but it's old (since M-52) and doesn't follow the same stack trace. Thread 0 (id: 24541) CRASHED [SIGSEGV @ 0x00000138 ] MAGIC SIGNATURE THREAD 0x00005b8bbe9d6e44 (chrome + 0x04b25e44 ) extensions::Extension::GetManifestData(std::string const&) const 0x00005b8bbea0011c (chrome + 0x04b4f11c ) extensions::IconsInfo::GetIcons(extensions::Extension const*) 0x00005b8bbedab540 (chrome + 0x04efa540 ) extensions::ChromeAppIcon::Reload() It seems possible that we're passing a null extension: void ChromeAppIcon::Reload() { const Extension* extension = GetExtension(); icon_ = base::MakeUnique<IconImage>( browser_context_, extension, IconsInfo::GetIcons(extension), resource_size_in_dip_, util::GetDefaultAppIcon(), this); UpdateIcon(); } // static const ExtensionIconSet& IconsInfo::GetIcons(const Extension* extension) { IconsInfo* info = static_cast<IconsInfo*>( extension->GetManifestData(keys::kIcons)); return info ? info->icons : g_empty_icon_set.Get(); } I believe that https://codereview.chromium.org/2900783003 fixed this at ToT by handling the NULL extension case properly, but that code is newer than M-60. CCing khmel to see if that sounds reasonable. CCing bhthompson about impact-- I don't know how to evaluate if this is worth addressing for 60. Here are crash statistics for the flow through GetIcons: https://crash.corp.google.com/browse?q=OMIT%20RECORD%20IF%20SUM(CrashedStackTrace.StackFrame.FunctionName%3D%27extensions%3A%3AIconsInfo%3A%3AGetIcons(extensions%3A%3AExtension%20const*)%27)%20%3D%200&ignore_case=false&enable_rewrite=true&omit_field_name=CrashedStackTrace.StackFrame.FunctionName&omit_field_value=extensions%3A%3AIconsInfo%3A%3AGetIcons(extensions%3A%3AExtension%20const*)&omit_field_opt=%3D#samplereports
,
Jul 14 2017
Requesting a merge to 60, but with a new commit applying the one line change in comment #1 instead of cherry-picking the entire change from ToT. Let me know if that's reasonable, and I'll send a small change for review on the 3112 branch.
,
Jul 14 2017
This bug requires manual review: We are only 10 days from stable. Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 14 2017
Comment 2 SGTM.
,
Jul 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/83629e3e5857bfc4cc93892bab59be593e6a5382 commit 83629e3e5857bfc4cc93892bab59be593e6a5382 Author: Justin TerAvest <teravest@chromium.org> Date: Fri Jul 14 22:35:18 2017 Fix segfault in ChromeAppIcon::Reload() The existing code has been steadily crashing since M56; callees don't expect to get a NULL extension value, leading to segfaults. This was fixed as part of a wider change in ToT: https://codereview.chromium.org/2900783003 Relevant crashes: https://crash.corp.google.com/browse?q=OMIT%20RECORD%20IF%20SUM(CrashedStackTrace.StackFrame.FunctionName%3D%27extensions%3A%3AIconsInfo%3A%3AGetIcons(extensions%3A%3AExtension%20const*)%27)%20%3D%200&ignore_case=false&enable_rewrite=true&omit_field_name=CrashedStackTrace.StackFrame.FunctionName&omit_field_value=extensions%3A%3AIconsInfo%3A%3AGetIcons(extensions%3A%3AExtension%20const*)&omit_field_opt=%3D#samplereports BUG= chromium:742521 Change-Id: I62074dd75f6de3d8091ea5c514743c1c356b85da Reviewed-on: https://chromium-review.googlesource.com/572049 Reviewed-by: Bernie Thompson <bhthompson@chromium.org> Cr-Commit-Position: refs/branch-heads/3112@{#611} Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897} [modify] https://crrev.com/83629e3e5857bfc4cc93892bab59be593e6a5382/chrome/browser/extensions/chrome_app_icon.cc
,
Jul 31 2017
No crashes have been reported after 60.0.3112.52.
,
Jan 22 2018
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by khmel@chromium.org
, Jul 13 2017