content::DownloadManagerDelegate subclasses accessed after destruction (android_webview, cast) |
|||
Issue descriptioncontent::DownloadManagerDelegate subclasses accessed after destruction in android_webview and cast Chromium platforms. content::~BrowserContext calls content::DownloadManagerImpl::Shutdown() content::DownloadManagerImpl::Shutdown() calls content::DownloadManagerDelegate::Shutdown() for its delegate_ That delegate_ is destroyed already in the CastBrowserContext::~CastBrowserContext() and AwBrowserContext::~AwBrowserContext(). The corresponding DownloadManagerDelegate::Shutdown methods are not overridden, so the methods are empty. This is probably why there's no that many crashes. This still seems a bit wrong construct to navigate the vptr for a destroyed object.
,
Aug 23 2017
gunsch -> halliwell
,
Aug 23 2017
yeah I think your reading is correct. not a big deal on webview though because BrowserContext is effectively global and is never deleted since there is no such thing as clean shutdown on android trying to work out lifetime of chrome's delegate, and it's... really complicated :|
,
Aug 23 2017
I would be surprised if it wasn't also effectively global in chrome/cast as well. AFAIK it's only on desktop where we have any hope of being able to shut down a Profile.
,
Aug 23 2017
incognito?
,
Aug 23 2017
Ah, yeah; the incognito delegate will likely get destroyed. :/
,
Aug 23 2017
apparently the correct thing to do is to call shutdown from content embedder before BrowserContext is destroyed: https://cs.chromium.org/chromium/src/chrome/browser/download/download_core_service_impl.cc?rcl=252178c30ceccb8f5be3e89f4ba421a4d27ddda2&l=137
,
Aug 23 2017
Or alternatively, set the delegate as UserData on the context..
,
Aug 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8aabb079725b5db1f0c1c6f3f63eb43cf9428fcb commit 8aabb079725b5db1f0c1c6f3f63eb43cf9428fcb Author: Bo Liu <boliu@chromium.org> Date: Thu Aug 24 04:17:17 2017 Fix DownloadManagerDelegate lifetime BrowserContext destructor still uses the DownloadManagerDelegate, so storing it as a member of a BrowserContext subclass is unsafe. This fixes the android_webview and chromecast, by converting it to a UserData on the BrowserContext. Bug: 758137 Change-Id: I2e35854085da2e3e14190fc2d04811e8483dd1b7 Reviewed-on: https://chromium-review.googlesource.com/629521 Reviewed-by: Richard Coles <torne@chromium.org> Reviewed-by: Luke Halliwell <halliwell@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Commit-Queue: Bo <boliu@chromium.org> Cr-Commit-Position: refs/heads/master@{#496947} [modify] https://crrev.com/8aabb079725b5db1f0c1c6f3f63eb43cf9428fcb/android_webview/browser/aw_browser_context.cc [modify] https://crrev.com/8aabb079725b5db1f0c1c6f3f63eb43cf9428fcb/android_webview/browser/aw_browser_context.h [modify] https://crrev.com/8aabb079725b5db1f0c1c6f3f63eb43cf9428fcb/android_webview/browser/aw_download_manager_delegate.h [modify] https://crrev.com/8aabb079725b5db1f0c1c6f3f63eb43cf9428fcb/chromecast/browser/cast_browser_context.cc [modify] https://crrev.com/8aabb079725b5db1f0c1c6f3f63eb43cf9428fcb/chromecast/browser/cast_browser_context.h [modify] https://crrev.com/8aabb079725b5db1f0c1c6f3f63eb43cf9428fcb/chromecast/browser/cast_download_manager_delegate.h [modify] https://crrev.com/8aabb079725b5db1f0c1c6f3f63eb43cf9428fcb/content/public/browser/browser_context.h
,
Aug 24 2017
|
|||
►
Sign in to add a comment |
|||
Comment 1 by kkinnu...@nvidia.com
, Aug 23 2017