PWA has out-of-scope (custom tab) treatment when scope is omitted |
|||
Issue descriptionChrome Version: 68.0.3420.1 OS: Android What steps will reproduce the problem? (1) Visit https://bit.ly/2rphBWP (2) Install the app. (3) Open the installed app. What is the expected result? App is displayed as a standalone window (with no title bar). What happens instead? App has a Chrome Custom Tab treatment as if the user had navigated out of scope. (Note: This is slightly different to the minimal-ui treatment, because it has an X button that actually does nothing when you tap it, because there is nothing to go back to.) Analysis Compare the following two apps: 1. No "scope": https://bit.ly/2rphBWP 2. scope ".": https://bit.ly/2HW0aHB The "no scope" version shows a CCT title bar, while the "scope ." version displays correctly. They should be identical. According to [1], if "scope" is omitted, it should default to ".", resolved against start URL. Since start URL is also ".", the scope should resolve to the same thing in both cases: the directory containing the manifest URL (which is the top-level directory of the app, in both cases, "https://killer-marmot.appspot.com/custom/<long-string>/"). I think the scope is resolving to something else in case (1), which is causing Chrome to think the start URL is out-of-scope, despite being installable. Doesn't affect desktop. [1] https://www.w3.org/TR/appmanifest/#scope-member
,
May 7 2018
We dont' do any caching based on name - the manifest urls would have to match. Potentially could be calculation of default scope. Assigning to Glenn for first look
,
May 7 2018
This does look like a server issue. Moving discussion to http://b/79353617
,
May 7 2018
Actually, I think I was wrong. The escaped scope of "https://killer-marmot.appspot.com/custom/eyJuYW1lIjogIktpbGxlciBNYXJtb3QiLCAic2hvcnRfbmFtZSI6ICJNYXJtb3QiLCAiZGlzcGxheSI6ICJzdGFuZGFsb25lIiwgInN0YXJ0X3VybCI6ICIuIn0%3D/" is getting passed to the server from Chrome, and then Chrome is (I think) just doing a string compare when it gets the WebAPK back and fails because the escaped scope doesn't match the unescaped starturl don't match. Chrome should either do more consistent escaping (or lack thereof) in ShortcutHelper.getScopeFromUrl() (https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java?q=getScopeFromUrl&l=606) or do a smarter URL comparison when it's checking if the start url is within the scope.
,
May 7 2018
Note that I strongly prefer the former solution (not escaping the scope url) both for consistency with the start url and because it could cause oneoffs.
,
May 8 2018
Oh, so you think the issue is the "=" being escaped to "%3D"? (So perhaps all of my above guesses were based on red herrings; because I generate a different URL for each manifest, some of them have "=" in them, and others don't.) However, note that the "scope ." version (which works correctly) does have a "=" in it. Having "=" be automatically escaped as "%3D" seems like a legitimate bug; "=" is not equivalent to "%3D" in a URL path (or query or fragment) [1] and "=" should remain as "=" when canonicalizing a URL. Ironically the server is using Python's base64.urlsafe_b64encode [2] because it is safe to put those strings in a URL. [1] https://url.spec.whatwg.org/#path-percent-encode-set [2] https://docs.python.org/2/library/base64.html#base64.urlsafe_b64encode
,
May 8 2018
I looked at the code for getScopeFromUrl. I think the problem is that while the URL standards (both RFC 2396 and URL Standard) reserve "=" in the path (that is, "=" and "%3D" have distinct meanings), this code unescapes and re-escapes the path, an operation that does not round-trip properly. 612 List<String> path = uri.getPathSegments(); // Unescapes path. ... 628 builder.path(scope_path); // Re-escapes path. So "=" round-trips to "%3D", while "%3D" round-trips "%3D". The correct behaviour is to not decode in the first place. Unfortunately, there is no Uri.getEncodedPathSegments, so you need to use Uri.getEncodedPath() then split on '/'. Then use Uri.Builder.encodedPath(). That should fix it. (IMHO this is bad API design; Uri and Uri.Builder should not be automatically decoding and encoding components, because it leads to unnecessary round-tripping and accidents like this.) Aside: This behaviour (default scope) *used to be* a specific implementation detail of WebAPKs, which is why it lives in the WebAPK code in Chrome. However, we have since updated the Manifest spec to codify this at the top level. So really, this logic should move from the WebAPK-specific Java code into the main manifest parser.
,
May 21 2018
I think the right solution to this bug is to subsume it into Issue 809837, which is part of the work to make the manifest parser assign the default scope. We need to do this to be spec-compliant. It's on my TODO list, but currently unprioritised; mgiuca, how soon do you want this looked at?
,
May 21 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/163757caa53023d9b35e8b7511d2613744ab74cc commit 163757caa53023d9b35e8b7511d2613744ab74cc Author: Peter Kotwicz <pkotwicz@chromium.org> Date: Mon May 21 01:36:07 2018 [Webapp] Don't do any escaping during default scope computation This CL fixes the URL formatting in ShortcutHelper#getScopeFromUrl(). Previously getScopeFromUrl() was decoding then re-encoding the path, which does not round-trip correctly as it encodes characters like '='. This bug was causing the WebAPK to think that the start URL was outside of the scope generated by ShortcutHelper#getScopeFromUrl() and to launch a CCT BUG=840271 TEST=ShortcutHelperTest Change-Id: Ibf63a8b13717c33ce6e8f113d58192a964e53d2d Reviewed-on: https://chromium-review.googlesource.com/1053491 Commit-Queue: Peter Kotwicz <pkotwicz@chromium.org> Reviewed-by: Bernhard Bauer <bauerb@chromium.org> Reviewed-by: Matt Giuca <mgiuca@chromium.org> Cr-Commit-Position: refs/heads/master@{#560217} [modify] https://crrev.com/163757caa53023d9b35e8b7511d2613744ab74cc/chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java [modify] https://crrev.com/163757caa53023d9b35e8b7511d2613744ab74cc/chrome/android/junit/src/org/chromium/chrome/browser/ShortcutHelperTest.java
,
May 22 2018
dominickn@: I actually have a CL mostly written which updates manifest_parser.cc The unfortunate part is that we need to keep the ShortcutHelper#getScopeFromUrl() because: - old shortcuts which were added to the homescreen may not have a scope - we create WebappInfo in WebappLauncherActivity (and WebappActivity) prior to native. We could split WebappInfo into a part which is computed prior to native being loaded and part which is computed after native is loaded but this is non trivial
,
May 23 2018
#10: hmm, I also have a CL mostly written that updates manifest_parser.cc (since Issue 809837 was assigned to me). I agree we'll need to keep getScopeFromUrl for old shortcuts, but we won't need to have any other scope computation aside from parsing the manifest. |
|||
►
Sign in to add a comment |
|||
Comment 1 by mgiuca@chromium.org
, May 7 2018