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

Issue 2620403002: Fix WebApkInstallService::IsInstallInProgress() crash. (Closed)

Created:
3 years, 11 months ago by Xi Han
Modified:
3 years, 11 months ago
Reviewers:
pkotwicz, gone
CC:
chromium-reviews, dominickn+watch_chromium.org, pkotwicz+watch_chromium.org, zpeng+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix WebApkInstallService::IsInstallInProgress() crash. The crash happens in a incognito page when a A2HS banner shows for a WebApp. It is caused since WebApkInstallServiceFactory didn't implemtent GetBrowserContextToUse() which returns a nullptr as default. In this CL, the same pointer of the original browser context is returned for the incognito page, so calling of WebApkInstallerService::Get() can return an null-empty instance of WebApkInstallerService. Crash Stacktrace: Thread 0 CRASHED [SIGSEGV @ 0x0000000c ] MAGIC SIGNATURE THREAD Stack Quality20%Show frame trust levels 0xd1af55a0 (libmonochrome.so -__tree:878 ) WebApkInstallService::IsInstallInProgress(GURL const&) 0xd1aefe43 (libmonochrome.so -shortcut_helper.cc:273 ) ShortcutHelper::IsWebApkInstalled(content::BrowserContext*, GURL const&, GURL const&) 0xd1a24ea3 (libmonochrome.so -app_banner_manager.cc:244 ) banners::AppBannerManager::PerformInstallableCheck() 0xd1abafd7 (libmonochrome.so -app_banner_manager_android.cc:180 ) banners::AppBannerManagerAndroid::PerformInstallableCheck() 0xd1a25c91 (libmonochrome.so -app_banner_manager.cc:239 ) banners::AppBannerManager::OnDidGetManifest(InstallableData const&) 0xd1a548d5 (libmonochrome.so -callback.h:85 ) InstallableManager::RunCallback(std::__ndk1::pair<InstallableParams, base::Callback<void (InstallableData const&), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> > const&, InstallableStatusCode) 0xd1a54a4d (libmonochrome.so -installable_manager.cc:246 ) InstallableManager::WorkOnTask() 0xd1a54d55 (libmonochrome.so -installable_manager.cc:288 ) InstallableManager::OnDidGetManifest(GURL const&, content::Manifest const&) BUG=679826 Review-Url: https://codereview.chromium.org/2620403002 Cr-Commit-Position: refs/heads/master@{#443293} Committed: https://chromium.googlesource.com/chromium/src/+/7ff526a78d1d5b3bd4757b66b7557d23e5d8bf0f

Patch Set 1 #

Patch Set 2 : Handle the incognito mode. #

Total comments: 6

Patch Set 3 : Returns the same browser context in the incognito mode. #

Patch Set 4 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+9 lines, -0 lines) Patch
M chrome/browser/android/webapk/webapk_install_service_factory.h View 1 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/android/webapk/webapk_install_service_factory.cc View 1 2 3 2 chunks +6 lines, -0 lines 0 comments Download

Messages

Total messages: 35 (21 generated)
Xi Han
Hi Peter, could you please take a look? Thanks!
3 years, 11 months ago (2017-01-11 20:19:55 UTC) #6
pkotwicz
I managed to reproduce the crash locally. I get this crash when I visit tests.peter.sh ...
3 years, 11 months ago (2017-01-11 21:41:34 UTC) #7
Xi Han
Thank you Peter for the suggestion, I added the implementation in. I noticed the incognito ...
3 years, 11 months ago (2017-01-12 14:47:27 UTC) #13
pkotwicz
https://codereview.chromium.org/2620403002/diff/120001/chrome/browser/android/shortcut_helper.cc File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/2620403002/diff/120001/chrome/browser/android/shortcut_helper.cc#newcode276 chrome/browser/android/shortcut_helper.cc:276: return service != nullptr && service->IsInstallInProgress(manifest_url); This is no ...
3 years, 11 months ago (2017-01-12 15:37:42 UTC) #14
Xi Han
PTAL, thanks! https://codereview.chromium.org/2620403002/diff/120001/chrome/browser/android/shortcut_helper.cc File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/2620403002/diff/120001/chrome/browser/android/shortcut_helper.cc#newcode276 chrome/browser/android/shortcut_helper.cc:276: return service != nullptr && service->IsInstallInProgress(manifest_url); On ...
3 years, 11 months ago (2017-01-12 15:50:36 UTC) #15
pkotwicz
https://codereview.chromium.org/2620403002/diff/120001/chrome/browser/android/webapk/webapk_install_service_factory.cc File chrome/browser/android/webapk/webapk_install_service_factory.cc (right): https://codereview.chromium.org/2620403002/diff/120001/chrome/browser/android/webapk/webapk_install_service_factory.cc#newcode38 chrome/browser/android/webapk/webapk_install_service_factory.cc:38: // The base class's implementation returns nullptr. I think ...
3 years, 11 months ago (2017-01-12 15:55:40 UTC) #16
Xi Han
PTAL, thanks! https://codereview.chromium.org/2620403002/diff/120001/chrome/browser/android/webapk/webapk_install_service_factory.cc File chrome/browser/android/webapk/webapk_install_service_factory.cc (right): https://codereview.chromium.org/2620403002/diff/120001/chrome/browser/android/webapk/webapk_install_service_factory.cc#newcode38 chrome/browser/android/webapk/webapk_install_service_factory.cc:38: // The base class's implementation returns nullptr. ...
3 years, 11 months ago (2017-01-12 16:25:24 UTC) #17
pkotwicz
LGTM
3 years, 11 months ago (2017-01-12 16:48:53 UTC) #18
Xi Han
Hi Dan, I need OWNERS review, could you please take a look? Thanks!
3 years, 11 months ago (2017-01-12 16:49:48 UTC) #20
pkotwicz
You should clarify your CL description. You should include a description of why the crash ...
3 years, 11 months ago (2017-01-12 16:49:56 UTC) #22
Xi Han
On 2017/01/12 16:49:56, pkotwicz wrote: > You should clarify your CL description. You should include ...
3 years, 11 months ago (2017-01-12 16:59:56 UTC) #26
gone
rs lgtm
3 years, 11 months ago (2017-01-12 18:10:49 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2620403002/160001
3 years, 11 months ago (2017-01-12 18:12:08 UTC) #32
commit-bot: I haz the power
3 years, 11 months ago (2017-01-12 18:17:56 UTC) #35
Message was sent while issue was closed.
Committed patchset #4 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/7ff526a78d1d5b3bd4757b66b755...

Powered by Google App Engine
This is Rietveld 408576698