New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user
Status: Fixed
Owner:
Closed: Nov 2014
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug-Security



Sign in to add a comment
Investigate lifetime of the NativeWindow parent in ExtensionUninstallDialog
Project Member Reported by sashab@chromium.org, Jul 25 2014 Back to list
Since the ExtensionUninstallDialog is not constructed until its image has loaded, there could be a lifetime issue where the parent window is closed in the time between when the ExtensionUninstallDialog is created and the app icon has been loaded. If the parent window is freed in this time, but ExtensionUninstallDialog is not, OnImageLoaded() will run with an already-freed parent_ variable and crash.

To fix this, we could add a WidgetObserver to the parent window to watch for when it closes, and then prevent the dialog from launching if it closes in this time. Alternatively, the dialog could be constructed in the constructor, OnClosed() can be overridden to call ExtensionUninstallCancelled(), and OnImageLoaded can simply call Show() on the widget to display it (however, this may need a refresh call to re-layout the dialog controls now that the icon has been changed).

Another solution again would be to make uninstall a sub-class of the install dialog, which just doesn't display the permissions and has slightly different text. However, it should first be investigated whether ExtensionInstallPrompt is also vulnerable to this problem.
 
Comment 1 by kalman@chromium.org, Jul 25 2014
OnImageLoaded is base::Bound to a WeakPtr of ExtensionUninstallDialog... it sounds like a good solution would be the WidgetObserver approach, and the Dialog deleting itself when the parent closes?

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/extensions/extension_uninstall_dialog.cc&sq=package:chromium&rcl=1406209044&l=97
Project Member Comment 2 by bugdroid1@chromium.org, Nov 4 2014
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ea87b86d5bd1abe26636abfc19af28e755da7585

commit ea87b86d5bd1abe26636abfc19af28e755da7585
Author: pkotwicz <pkotwicz@chromium.org>
Date: Tue Nov 04 22:42:33 2014

Cancel uninstall if the uninstall dialog's parent window is destroyed prior to
the uninstall dialog showing.

BUG= 424999 ,  397396 
TEST=ExtensionUninstallDialogViewBrowserTest.TrackParentWindowDestruction

Review URL: https://codereview.chromium.org/697023002

Cr-Commit-Position: refs/heads/master@{#302691}

[modify] https://chromium.googlesource.com/chromium/src.git/+/ea87b86d5bd1abe26636abfc19af28e755da7585/chrome/browser/extensions/extension_uninstall_dialog.cc
[modify] https://chromium.googlesource.com/chromium/src.git/+/ea87b86d5bd1abe26636abfc19af28e755da7585/chrome/browser/extensions/extension_uninstall_dialog.h
[modify] https://chromium.googlesource.com/chromium/src.git/+/ea87b86d5bd1abe26636abfc19af28e755da7585/chrome/browser/ui/cocoa/extensions/extension_uninstall_dialog_cocoa.mm
[modify] https://chromium.googlesource.com/chromium/src.git/+/ea87b86d5bd1abe26636abfc19af28e755da7585/chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc
[add] https://chromium.googlesource.com/chromium/src.git/+/ea87b86d5bd1abe26636abfc19af28e755da7585/chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc
[modify] https://chromium.googlesource.com/chromium/src.git/+/ea87b86d5bd1abe26636abfc19af28e755da7585/chrome/chrome_tests.gypi

Labels: -Type-Bug Type-Bug-Security Security_Severity-High Restrict-View-SecurityTeam
Status: Fixed
Project Member Comment 4 by ClusterFuzz, Nov 5 2014
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify Merge-NA
Project Member Comment 5 by bugdroid1@chromium.org, Nov 11 2014
Labels: merge-merged-2171
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6919459fb7f674aaf05277ebe2fe12e746d4a3f9

commit 6919459fb7f674aaf05277ebe2fe12e746d4a3f9
Author: Peter Kotwicz <pkotwicz@google.com>
Date: Tue Nov 11 16:46:12 2014

[Merge] Cancel uninstall if the uninstall dialog's parent window is destroyed prior to the uninstall dialog showing.

BUG= 424999 ,  397396 
TEST=ExtensionUninstallDialogViewBrowserTest.TrackParentWindowDestruction
TBR=pkotwicz

Review URL: https://codereview.chromium.org/697023002

Review URL: https://codereview.chromium.org/711343003

Cr-Commit-Position: refs/branch-heads/2171@{#402}
Cr-Branched-From: 267aeeb8d85c8503a7fd12bd14654b8ea78d3974-refs/heads/master@{#297060}

[modify] https://chromium.googlesource.com/chromium/src.git/+/6919459fb7f674aaf05277ebe2fe12e746d4a3f9/chrome/browser/extensions/extension_uninstall_dialog.cc
[modify] https://chromium.googlesource.com/chromium/src.git/+/6919459fb7f674aaf05277ebe2fe12e746d4a3f9/chrome/browser/extensions/extension_uninstall_dialog.h
[modify] https://chromium.googlesource.com/chromium/src.git/+/6919459fb7f674aaf05277ebe2fe12e746d4a3f9/chrome/browser/ui/cocoa/extensions/extension_uninstall_dialog_cocoa.mm
[modify] https://chromium.googlesource.com/chromium/src.git/+/6919459fb7f674aaf05277ebe2fe12e746d4a3f9/chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc
[add] https://chromium.googlesource.com/chromium/src.git/+/6919459fb7f674aaf05277ebe2fe12e746d4a3f9/chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc
[modify] https://chromium.googlesource.com/chromium/src.git/+/6919459fb7f674aaf05277ebe2fe12e746d4a3f9/chrome/chrome_tests.gypi

Labels: -Merge-NA Release-0-M39
Project Member Comment 7 by ClusterFuzz, Nov 12 2014
Labels: -Release-0-M39
This bug is a regression and does not impact stable. Removing incorrectly added Release-0-M39 label.

- Your friendly ClusterFuzz
Labels: Security_Impact-Stable Release-0-M39
Project Member Comment 9 by ClusterFuzz, Feb 11 2015
Labels: -Restrict-View-SecurityNotify
Bulk update: removing view restriction from closed bugs.
Project Member Comment 10 by sheriffbot@chromium.org, Oct 1 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member Comment 11 by sheriffbot@chromium.org, Oct 2 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: allpublic
Sign in to add a comment