New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 762388 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Long OOO (go/where-is-mgiuca)
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Web Share Target: If manifest url is invalid, crashes

Project Member Reported by mgiuca@chromium.org, Sep 6 2017

Issue description

DCHECK fail in share_service_impl.cc:155 if any manifest_url from the Preferences 'profile.web_share.visited_targets' key is blank or invalid.

I don't know how this can happen, but it somehow happened to my Preferences file. We shouldn't crash when the users Preferences is corrupted, if possible, so let's just continue gracefully.
 

Comment 1 by mgiuca@chromium.org, Sep 25 2017

Cc: sa...@chromium.org pkotw...@chromium.org
Thanks to sammc@, I tracked down what was causing the empty URL to be stored in prefs. If the share target was added via an app banner, rather than the "Add to Desktop" menu item, it will have no URL.

Steps to reproduce:
1. Run Chrome with --enable-experimental-web-platform-features --enable-features=AppBanners --bypass-app-banner-engagement-checks
2. Visit: https://wicg.github.io/web-share-target/demos/sharetarget.html
3. Should see an app install banner. Click "Add" to install it through the banner.
4. Visit: https://wicg.github.io/web-share/demos/share.html
5. Click "Share".

Crash:

[1043:1043:0925/184139.077810:FATAL:share_service_impl.cc(150)] Check failed: manifest_url.is_valid(). 

The root cause is that BookmarkAppHelper::CreateFromAppBanner calls BookmarkAppHelper::OnDidGetManifest with an empty GURL(), and that gets passed down and stored in the manifest.

History: https://crrev.com/403523 introduced a new GURL manifest_url parameter to BookmarkAppHelper::OnDidGetManifest and passes it around a lot. Later, we used this parameter to store the manifest URL for Web Share Target.

It turns out that in a number of places, an empty GURL() is passed in:

- BookmarkAppHelper::CreateFromAppBanner
- ManifestManagerHost::RenderFrameDeleted
- ManifestManager::GetManifest
- ManifestManager::ResolveCallbacks

I believe that none of these places should pass an empty GURL(); it's just wrong to have a parameter called "manifest_url" that's empty when the manifest itself is valid.

Now I think we should:
1. https://crrev.com/c/652011 avoid crashing when the URL in the prefs is blank (basically, regardless of what is written to prefs, code should not assume prefs are valid).
2. Migrate the pref data to remove these bad entries. (Maybe not necessary since these should only be written when the --enable-experimental-web-platform-features flag is enabled.)
3. Add a DCHECK in BookmarkAppHelper::OnDidGetManifest that GURL must be valid if the manifest is valid.
4. Fix all the call sites to pass the true manifest URL (possibly hard).

Comment 2 by mgiuca@chromium.org, Sep 26 2017

Looks like the last 3 call sites (ManifestManagerHost::RenderFrameDeleted, ManifestManager::GetManifest, ManifestManager::ResolveCallbacks) are either passing empty GURL because it's legitimately a missing manifest, or they've since been fixed.

It's just BookmarkAppHelper::CreateFromAppBanner that's a problem.
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 26 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e0739ed09afe6042c36e9a76da3bc58431b75b71

commit e0739ed09afe6042c36e9a76da3bc58431b75b71
Author: Matt Giuca <mgiuca@chromium.org>
Date: Tue Sep 26 09:28:22 2017

Web Share Target (desktop): Fail gracefully if manifest URL is blank.

Previously it would DCHECK if any share target in the Preferences file
had a blank or invalid manifest URL, which could be caused if the share
target was previously installed using an app banner. Since this is
corrupted user data, we should recover, not DCHECK, in these cases.

Also adds a DCHECK to catch cases that would write this corrupted data
into the preferences. This doesn't yet fix the issue, but it does report
it to developers. The DCHECK only fires if the manifest has a
share_target and the --enable-experimental-web-platform-features flag is
enabled (so it won't be too annoying).

Bug:  762388 
Change-Id: Ibd800dc529510d4755068f7bb0c55f8ca083bc7e
Reviewed-on: https://chromium-review.googlesource.com/652011
Commit-Queue: Matt Giuca <mgiuca@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504325}
[modify] https://crrev.com/e0739ed09afe6042c36e9a76da3bc58431b75b71/chrome/browser/webshare/share_service_impl.cc
[modify] https://crrev.com/e0739ed09afe6042c36e9a76da3bc58431b75b71/chrome/browser/webshare/share_service_impl_unittest.cc
[modify] https://crrev.com/e0739ed09afe6042c36e9a76da3bc58431b75b71/chrome/browser/webshare/share_target_pref_helper.cc

Comment 4 by mgiuca@chromium.org, Sep 27 2017

That CL I landed (r504325) makes the steps to reproduce the crash far more direct (in a DCHECK-enabled build on Linux):

1. Run Chrome with --enable-experimental-web-platform-features --enable-features=AppBanners --bypass-app-banner-engagement-checks
2. Visit: https://wicg.github.io/web-share-target/demos/sharetarget.html
3. Should see an app install banner. Click "Add" to install it through the banner.
*crash*

[409:409:0927/132726.288965:FATAL:share_target_pref_helper.cc(30)] Check failed: manifest_url.is_valid(). 

I've got another CL to pass the correct URL through: http://crrev.com/c/686084
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9e58d5c52840a09b62ec32afd0f3153c0be21b70

commit 9e58d5c52840a09b62ec32afd0f3153c0be21b70
Author: Matt Giuca <mgiuca@chromium.org>
Date: Thu Sep 28 01:54:54 2017

Fix invalid Web share targets when app added via install banner.

Apps added via the install banner were being installed with an empty
manifest URL. The correct manifest URL is now passed through to
BookmarkAppHelper::OnDidGetManifest so the share target is recorded
correctly.

Bug:  762388 
Change-Id: I705c1f067946d3e7349c0d3be8e06c77b096be72
Reviewed-on: https://chromium-review.googlesource.com/686084
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Finnur Thorarinsson <finnur@chromium.org>
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504864}
[modify] https://crrev.com/9e58d5c52840a09b62ec32afd0f3153c0be21b70/chrome/browser/banners/app_banner_infobar_delegate_desktop.cc
[modify] https://crrev.com/9e58d5c52840a09b62ec32afd0f3153c0be21b70/chrome/browser/banners/app_banner_infobar_delegate_desktop.h
[modify] https://crrev.com/9e58d5c52840a09b62ec32afd0f3153c0be21b70/chrome/browser/banners/app_banner_manager_desktop.cc
[modify] https://crrev.com/9e58d5c52840a09b62ec32afd0f3153c0be21b70/chrome/browser/extensions/bookmark_app_helper.cc
[modify] https://crrev.com/9e58d5c52840a09b62ec32afd0f3153c0be21b70/chrome/browser/extensions/bookmark_app_helper.h
[modify] https://crrev.com/9e58d5c52840a09b62ec32afd0f3153c0be21b70/chrome/browser/extensions/bookmark_app_helper_unittest.cc

Comment 6 by mgiuca@chromium.org, Sep 28 2017

Status: Fixed (was: Started)

Sign in to add a comment