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

Issue 840271 link

Starred by 4 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

PWA has out-of-scope (custom tab) treatment when scope is omitted

Project Member Reported by mgiuca@chromium.org, May 7 2018

Issue description

Chrome 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
 
UPDATE: Actually that repro step doesn't seem to work. (Sorry, I updated the name used in the test after trying it to make it more clear but I broke the repro.)

Instead, just use this:
https://killer-marmot.appspot.com/custom/

That's exactly the same as https://bit.ly/2rphBWP in the bug report, except it has the standard name "Killer Marmot" and short name "Marmot". Some how, this is causing it to show the CCT as described above. If you change either the long or short name, it still shows the CCT, but if you change BOTH the long and short name, it will correctly show standalone.

Note: Even if you uninstall all other apps, it still shows the CCT.

Why? I suspect this is a server-side issue. The server is assuming that an app with the name "Killer Marmot" and short name "Marmot" has a scope of https://killer-marmot.appspot.com/web/ (or something) -- which it has seen hundreds of times before as we regularly test on there. But by changing the manifest to a new location but keeping the name the same, it is still assuming the old scope. Explicitly specifying the scope doesn't have the problem.
Owner: hartma...@chromium.org
Status: Assigned (was: Untriaged)
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
This does look like a server issue. Moving discussion to http://b/79353617
Cc: hartma...@chromium.org
Owner: pkotw...@chromium.org
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.
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.
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
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.
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?
Project Member

Comment 9 by bugdroid1@chromium.org, 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

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
#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