Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(503)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 12210107: Fix issue 174250, crash when reloading packaged app. (Closed)

Created:
7 years, 10 months ago by jeremya
Modified:
7 years, 4 months ago
CC:
chromium-reviews, Aaron Boodman, jeremya+watch_chromium.org, chromium-apps-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

Fix issue 174250, crash when reloading packaged app. When a shell window was closed, it wasn't removed from the registry immediately (it was removed in a callback from OnNativeClose, which happens a bit later.) This meant that when the app was loaded again, the window was still in the registry with a stale pointer to the old extension. ExtensionSettingsHandler was tickling this by iterating shell windows on extension load. BUG=174250 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182194

Patch Set 1 #

Total comments: 2

Patch Set 2 : also immediately remove from registry in CloseContents and APP_TERMINATING #

Patch Set 3 : null out extension_ after Close(). #

Patch Set 4 : fix use-after-free #

Patch Set 5 : fix mac #

Patch Set 6 : test #

Total comments: 4

Patch Set 7 : comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+68 lines, -15 lines) Patch
M chrome/browser/extensions/platform_app_browsertest.cc View 1 2 3 4 5 6 3 chunks +20 lines, -1 line 0 comments Download
M chrome/browser/extensions/shell_window_registry.cc View 1 chunk +3 lines, -2 lines 0 comments Download
M chrome/browser/ui/cocoa/extensions/native_app_window_cocoa.mm View 1 2 3 4 5 6 2 chunks +9 lines, -1 line 0 comments Download
M chrome/browser/ui/extensions/shell_window.h View 1 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/ui/extensions/shell_window.cc View 1 2 3 4 chunks +22 lines, -6 lines 0 comments Download
M chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc View 1 2 3 2 chunks +9 lines, -5 lines 0 comments Download

Messages

Total messages: 18 (0 generated)
jeremya
7 years, 10 months ago (2013-02-10 23:17:19 UTC) #1
benwells
lgtm https://codereview.chromium.org/12210107/diff/1/chrome/browser/ui/extensions/shell_window.cc File chrome/browser/ui/extensions/shell_window.cc (right): https://codereview.chromium.org/12210107/diff/1/chrome/browser/ui/extensions/shell_window.cc#newcode576 chrome/browser/ui/extensions/shell_window.cc:576: if (extension_ == unloaded_extension) { Are there other ...
7 years, 10 months ago (2013-02-10 23:19:36 UTC) #2
jeremya
https://codereview.chromium.org/12210107/diff/1/chrome/browser/ui/extensions/shell_window.cc File chrome/browser/ui/extensions/shell_window.cc (right): https://codereview.chromium.org/12210107/diff/1/chrome/browser/ui/extensions/shell_window.cc#newcode576 chrome/browser/ui/extensions/shell_window.cc:576: if (extension_ == unloaded_extension) { On 2013/02/10 23:19:36, benwells ...
7 years, 10 months ago (2013-02-10 23:58:44 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/12210107/5001
7 years, 10 months ago (2013-02-10 23:58:57 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/12210107/2005
7 years, 10 months ago (2013-02-11 01:11:23 UTC) #5
commit-bot: I haz the power
The commit queue went berserk retrying too often for a seemingly flaky test. Builder is ...
7 years, 10 months ago (2013-02-11 05:46:31 UTC) #6
jeremya
This was committed as http://crrev.com/181666, but apparently the bot didn't pick it up.
7 years, 10 months ago (2013-02-11 06:43:08 UTC) #7
jeremya
updated to make Close() asynchronous on GTK and Cocoa to match behaviour on Windows and ...
7 years, 10 months ago (2013-02-12 07:59:01 UTC) #8
jeremya
updated to make Close() asynchronous on GTK and Cocoa to match behaviour on Windows and ...
7 years, 10 months ago (2013-02-12 07:59:32 UTC) #9
Nico
Is it possible to write a test for this?
7 years, 10 months ago (2013-02-12 16:24:09 UTC) #10
jeremya
Test added, though I couldn't make it crash on GTK/ASan, only chromeos=1 ASan, and I'm ...
7 years, 10 months ago (2013-02-12 22:28:22 UTC) #11
Nico
cocoa lgtm There's a chromeos asan builder http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20ASAN%20Builder and there are testers too. Thanks for ...
7 years, 10 months ago (2013-02-13 01:22:32 UTC) #12
jeremya
https://codereview.chromium.org/12210107/diff/10016/chrome/browser/extensions/platform_app_browsertest.cc File chrome/browser/extensions/platform_app_browsertest.cc (right): https://codereview.chromium.org/12210107/diff/10016/chrome/browser/extensions/platform_app_browsertest.cc#newcode943 chrome/browser/extensions/platform_app_browsertest.cc:943: // Now tell the app to reload itself On ...
7 years, 10 months ago (2013-02-13 01:33:18 UTC) #13
Evan Stade
gtk lgtm
7 years, 10 months ago (2013-02-13 02:57:36 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/12210107/2014
7 years, 10 months ago (2013-02-13 05:26:29 UTC) #15
commit-bot: I haz the power
Change committed as 182194
7 years, 10 months ago (2013-02-13 11:53:00 UTC) #16
koz (OOO until 15th September)
7 years, 4 months ago (2013-07-29 00:13:47 UTC) #17
koz (OOO until 15th September)
7 years, 4 months ago (2013-07-29 00:13:57 UTC) #18
Message was sent while issue was closed. 
er, that is lgtm

Powered by Google App Engine
This is Rietveld 408576698