Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
24177003:
Fixed crash when chrome.runtime.reload() is called with app windows open (Closed)
|
Created:
7 years, 3 months ago by tmdiep Modified:
7 years, 2 months ago CC:
chromium-reviews, tfarina, scheib+watch_chromium.org, extensions-reviews_chromium.org, chromium-apps-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionFixed crash when chrome.runtime.reload() is called with app windows open
app_window_contents.cc:
Use host_->extension_id() as host_->extension() is freed when the app
is reloaded. This fixes the crash.
extension_process_manager.cc/h:
Unregister render view hosts when the old extension instance is unloaded,
since the background page data is erased. This avoids a DCHECK failure in
DecrementLazyKeepaliveCount() due to an excess decrement when the old
render view is destroyed.
extension_service.h:
Changed potentially misleading parameter name, as the function uses the
extension ID, not the name.
BUG=295266
TEST=See bug description for steps to reproduce crash
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=225360
Patch Set 1 #
Total comments: 5
Patch Set 2 : Remove RenderViewHosts from all_extension_views on unload instead of re-registering them when the n… #
Total comments: 4
Patch Set 3 : Fixed compilation failures #Patch Set 4 : Fixed compilation failures #
Messages
Total messages: 20 (0 generated)
https://codereview.chromium.org/24177003/diff/1/apps/app_window_contents.cc File apps/app_window_contents.cc (right): https://codereview.chromium.org/24177003/diff/1/apps/app_window_contents.cc#n... apps/app_window_contents.cc:99: host_->extension_id(), huh, I was just looking at this today due to https://code.google.com/p/chromium/issues/detail?id=265922. TBH I don't like the way there is extension_id, which is to be used when extension is NULL. Feels dodgy. https://codereview.chromium.org/24177003/diff/1/chrome/browser/extensions/ext... File chrome/browser/extensions/extension_process_manager.cc (right): https://codereview.chromium.org/24177003/diff/1/chrome/browser/extensions/ext... chrome/browser/extensions/extension_process_manager.cc:609: ReregisterRenderViewHosts(extension->id()); This makes me uncomfortable. The render view hosts will be destroyed very soon which could make the lazy count go to zero and get unloaded. But worse than that, they are really for a different extension which is now dead. Can we instead clear all_extension_views_ when the extension is unloaded, or will that have other side effects? Unloading / reloading has a few flaky problems which need to be fixed. E.g. https://code.google.com/p/chromium/issues/detail?id=174250
TL;DR - I might just sit on this patch, since there are general problems with reloading to sort out. It will prevent check in of work for the notification issue though: https://code.google.com/p/chromium/issues/detail?id=276990 https://codereview.chromium.org/24177003/diff/1/apps/app_window_contents.cc File apps/app_window_contents.cc (right): https://codereview.chromium.org/24177003/diff/1/apps/app_window_contents.cc#n... apps/app_window_contents.cc:99: host_->extension_id(), On 2013/09/20 07:35:31, benwells wrote: > huh, I was just looking at this today due to > https://code.google.com/p/chromium/issues/detail?id=265922. > > TBH I don't like the way there is extension_id, which is to be used when > extension is NULL. Feels dodgy. Darn, if I had waited a bit longer this could have been fixed for me :) If the ShellWindow sets the extension pointer to NULL when the extension unloads, perhaps we can check and not forward the message to the render view if it is not necessary to, since the window is closing. https://codereview.chromium.org/24177003/diff/1/chrome/browser/extensions/ext... File chrome/browser/extensions/extension_process_manager.cc (right): https://codereview.chromium.org/24177003/diff/1/chrome/browser/extensions/ext... chrome/browser/extensions/extension_process_manager.cc:609: ReregisterRenderViewHosts(extension->id()); On 2013/09/20 07:35:31, benwells wrote: > This makes me uncomfortable. The render view hosts will be destroyed very soon > which could make the lazy count go to zero and get unloaded. But worse than > that, they are really for a different extension which is now dead. Incrementing the render view hosts actually prevents the lazy count going to zero. When the extension loading begins, the count is incremented (and later decremented when the loading is complete). Without incrementing to account for the "old" render views, they will decrement the count to zero when they are deregistered and trigger the timeout for suspending the extension. It doesn't unload because an increment comes in soon due to some other network-related activity and therefore cancels the suspension. > Can we instead clear all_extension_views_ when the extension is unloaded, or > will that have other side effects? This sounds ok, but I'm not confident enough to say what side effects this will have. We do need to fix this count somehow though. > Unloading / reloading has a few flaky problems which need to be fixed. E.g. > https://code.google.com/p/chromium/issues/detail?id=174250 I noticed. I got the impression that the ExtensionService::ReloadExtension() function is heavy-handed - it unloads, destroys everything and initiates reloading synchronously. Meanwhile there is a lot of code still lurking around either with a dangling extension pointer or null pointer (and see a lot of usage of extension objects which don't check for null). Just feels like the whole process needs to be analysed and sequenced properly, rather than applying sporadic bandaids whenever we encounter a crash. My 2c :)
https://codereview.chromium.org/24177003/diff/1/chrome/browser/extensions/ext... File chrome/browser/extensions/extension_process_manager.cc (right): https://codereview.chromium.org/24177003/diff/1/chrome/browser/extensions/ext... chrome/browser/extensions/extension_process_manager.cc:609: ReregisterRenderViewHosts(extension->id()); On 2013/09/20 11:07:36, tmdiep wrote: > On 2013/09/20 07:35:31, benwells wrote: > > This makes me uncomfortable. The render view hosts will be destroyed very soon > > which could make the lazy count go to zero and get unloaded. But worse than > > that, they are really for a different extension which is now dead. > > Incrementing the render view hosts actually prevents the lazy count going to > zero. When the extension loading begins, the count is incremented (and later > decremented when the loading is complete). Without incrementing to account for > the "old" render views, they will decrement the count to zero when they are > deregistered and trigger the timeout for suspending the extension. It doesn't > unload because an increment comes in soon due to some other network-related > activity and therefore cancels the suspension. > > > Can we instead clear all_extension_views_ when the extension is unloaded, or > > will that have other side effects? > > This sounds ok, but I'm not confident enough to say what side effects this will > have. We do need to fix this count somehow though. > > > Unloading / reloading has a few flaky problems which need to be fixed. E.g. > > https://code.google.com/p/chromium/issues/detail?id=174250 > > I noticed. I got the impression that the ExtensionService::ReloadExtension() > function is heavy-handed - it unloads, destroys everything and initiates > reloading synchronously. Meanwhile there is a lot of code still lurking around > either with a dangling extension pointer or null pointer (and see a lot of usage > of extension objects which don't check for null). > > Just feels like the whole process needs to be analysed and sequenced properly, > rather than applying sporadic bandaids whenever we encounter a crash. My 2c :) That was the conclusion I came to today as well. Let's talk about it on Monday.
Left use of host_->extension_id(). Removed RenderViewHosts from all_extension_views_ when the old extension instance is unloaded instead of re-registering them when the new extension instance is loaded. This sends out the chrome::NOTIFICATION_EXTENSION_VIEW_UNREGISTERED notification sooner. The side effects of this appears to be: - The old render view process is removed from the Chrome task manager earlier - Developer private API receives notification of the view unregistered sooner
https://codereview.chromium.org/24177003/diff/9001/chrome/browser/extensions/... File chrome/browser/extensions/extension_process_manager.cc (right): https://codereview.chromium.org/24177003/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/extension_process_manager.cc:791: OnRenderViewHostUnregistered(profile, it->first); Would this have been sent out before when unloading?
https://codereview.chromium.org/24177003/diff/9001/chrome/browser/extensions/... File chrome/browser/extensions/extension_process_manager.cc (right): https://codereview.chromium.org/24177003/diff/9001/chrome/browser/extensions/... chrome/browser/extensions/extension_process_manager.cc:791: OnRenderViewHostUnregistered(profile, it->first); On 2013/09/23 05:38:23, benwells wrote: > Would this have been sent out before when unloading? This gets sent when the render view process is destroyed - so it would have been sent before when that old render view process is unregistered. If we don't broadcast the notification when removing the RenderViewHost from all_extension_views_, Chrome will crash badly with the Chrome task manager open.
lgtm but please wait for mpcomplete to have a look. Matt may prefer it with the re-registering method, or some other way.
For the ExtensionProcessManager change, maybe the bigger issue is that the views stay open when the extension is unloaded. What happens exactly? Do the views get reloaded as well? https://codereview.chromium.org/24177003/diff/9001/apps/app_window_contents.cc File apps/app_window_contents.cc (right): https://codereview.chromium.org/24177003/diff/9001/apps/app_window_contents.c... apps/app_window_contents.cc:99: host_->extension_id(), Rather than patching this here, could you change ExtensionHost to hold a scoped_refptr to the Extension object? That will prevent a use-after-free in case we access the extension object elsewhere. (Though I think the deeper bug is that the ExtensionHost isn't notified when the Extension object is freed. Extension should outlive ExtensionHost.)
> For the ExtensionProcessManager change, maybe the bigger issue is that the views > stay open when the extension is unloaded. What happens exactly? Do the views get > reloaded as well? Yes, the views stay alive briefly after the old extension instance is unloaded. They will then be reopened after the new extension instance is loaded. The ExtensionHost (background page) is actually destroyed synchronously during ExtensionService::ReloadExtension. The old Extension instance is destroyed asynchronously in the IO thread, when the new Extension instance is added in ExtensionInfoMap::AddExtension (the info map has the last reference). So the view outlives both ExtensionHost and Extension. https://codereview.chromium.org/24177003/diff/9001/apps/app_window_contents.cc File apps/app_window_contents.cc (right): https://codereview.chromium.org/24177003/diff/9001/apps/app_window_contents.c... apps/app_window_contents.cc:99: host_->extension_id(), On 2013/09/23 21:42:31, Matt Perry wrote: > Rather than patching this here, could you change ExtensionHost to hold a > scoped_refptr to the Extension object? That will prevent a use-after-free in > case we access the extension object elsewhere. (Though I think the deeper bug is > that the ExtensionHost isn't notified when the Extension object is freed. > Extension should outlive ExtensionHost.) The host_ pointer is an apps::ShellWindow. The ShellWindow does receive the extension unloaded notification. The patch submitted for this related issue sets the extension pointer to null to prevent use after free, but it has been reverted: https://code.google.com/p/chromium/issues/detail?id=174250 After reloading the app, AppWindowContents::NativeWindowChanged is called as a result of: 1. Widget::Close() 2. Widget::OnNativeWidgetVisibilityChanged(visible: false) 3. Widget::OnNativeWidgetActivationChanged(active: false) The third call is the cause of the crash. I think we can check whether the extension is null and not forward the message to the RenderViewHost.
On 2013/09/24 00:33:47, tmdiep wrote: > > For the ExtensionProcessManager change, maybe the bigger issue is that the > views > > stay open when the extension is unloaded. What happens exactly? Do the views > get > > reloaded as well? > > Yes, the views stay alive briefly after the old extension instance is unloaded. > They will then be reopened after the new extension instance is loaded. > > The ExtensionHost (background page) is actually destroyed synchronously during > ExtensionService::ReloadExtension. The old Extension instance is destroyed > asynchronously in the IO thread, when the new Extension instance is added in > ExtensionInfoMap::AddExtension (the info map has the last reference). So the > view outlives both ExtensionHost and Extension. > > https://codereview.chromium.org/24177003/diff/9001/apps/app_window_contents.cc > File apps/app_window_contents.cc (right): > > https://codereview.chromium.org/24177003/diff/9001/apps/app_window_contents.c... > apps/app_window_contents.cc:99: host_->extension_id(), > On 2013/09/23 21:42:31, Matt Perry wrote: > > Rather than patching this here, could you change ExtensionHost to hold a > > scoped_refptr to the Extension object? That will prevent a use-after-free in > > case we access the extension object elsewhere. (Though I think the deeper bug > is > > that the ExtensionHost isn't notified when the Extension object is freed. > > Extension should outlive ExtensionHost.) > > The host_ pointer is an apps::ShellWindow. The ShellWindow does receive the > extension unloaded notification. The patch submitted for this related issue sets > the extension pointer to null to prevent use after free, but it has been > reverted: > https://code.google.com/p/chromium/issues/detail?id=174250 > > After reloading the app, AppWindowContents::NativeWindowChanged is called as a > result of: > 1. Widget::Close() > 2. Widget::OnNativeWidgetVisibilityChanged(visible: false) > 3. Widget::OnNativeWidgetActivationChanged(active: false) > > The third call is the cause of the crash. I think we can check whether the > extension is null and not forward the message to the RenderViewHost. As an aside, I'd like to move to a model where the unload waits on all the shell windows being closed, however this is a significant change to how unloading works. Before tackling that change I think extension service needs some refactoring, which I've started looking at (it is also needed for app shell).
OK, it sounds like we all agree there are bigger issues, but this change at least fixes the crash. LGTM.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tmdiep@chromium.org/24177003/9001
Sorry for I got bad news for ya. Compile failed with a clobber build on android_clang_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tmdiep@chromium.org/24177003/48001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tmdiep@chromium.org/24177003/59001
Retried try job too often on linux_aura for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tmdiep@chromium.org/24177003/59001
Message was sent while issue was closed.
Change committed as 225360 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
