New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 742521 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Segfault in extensions::Extension::GetManifestData

Project Member Reported by teravest@chromium.org, Jul 13 2017

Issue description

Observation: 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
 

Comment 1 by khmel@chromium.org, Jul 13 2017

I think we can just merge one line:

>>
 extension ? IconsInfo::GetIcons(extension) : ExtensionIconSet(),
>>
This should be safe. Crash itself looks old, starting from M56.
Labels: -Pri-3 Merge-Request-60 Pri-2
Owner: teravest@chromium.org
Status: Assigned (was: Untriaged)
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.
Project Member

Comment 3 by sheriffbot@chromium.org, Jul 14 2017

Labels: -Merge-Request-60 Hotlist-Merge-Review Merge-Review-60
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
Labels: -Hotlist-Merge-Review -Merge-Review-60 Merge-Approved-60
Comment 2 SGTM.
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 14 2017

Labels: -merge-approved-60 merge-merged-3112
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

Status: Fixed (was: Assigned)
No crashes have been reported after 60.0.3112.52.

Comment 7 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Sign in to add a comment