1454228 - PWA fails launching with "Insecure connection"

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P1)

Description

[Tatsuyuki Ishi]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180323154952

Steps to reproduce:

Launch a PWA from home screen.

Probably first bad build number: 20180415000619


Actual results:

A toast "Insecure connection" was shown. The PWA was opened in a new tab instead of being opened immersively.


Expected results:

It should open full screen and not as a tab.

Comment 1

[Jan Henning [:JanH]]

Can you give a link to a PWA that exhibits that behaviour?

Comment 2

[Tatsuyuki Ishi]

Please try https://hn.nuxtjs.org/.

Comment 3

[Oana Horvath [:ohorvath - out of office]]

Can confirm this issue on the latest Nightly build (2018-04-16), with any PWA: twitter.com, pwa.rocks,ft.com.
Devices:
Huawei P9 Lite (Android 6.0)
HTC 10 (Android 8.0)

Comment 4

[Ryan VanderMeulen [:RyanVM]]

I see this with GMail too. We can't ship with this bug.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Dale, can you please help find an owner for this?

Comment 6

[Dale Harvey (:daleharvey)]

Yup noticed it as well, seemed like an upgrade issue, However I got taken off this feature / area quite a while ago, hopefully Andreas can find someone?

Comment 7

[Andreas Bovens [:abovens]]

This is because the start_url is loaded, but then immediately thereafter, a redirect happens. That's the reason why the PWA is opened in the browser instead. 

The same behavior can be observed in gotransit.com

Personally, I'm not sure what is best here... I do think it's a little strange to redirect immediately after launching the start_url: why not point to the right location from the manifest? That said, redirects within the same domain are fine, and perhaps the "Insecure connection" string should be more precise.

Comment 8

[Andreas Bovens [:abovens]]

Pinging Snorp and Nevin for their take.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

This behavior also shows on https://pinafore.social as well.  It has a start_url of "/?pwa=true".  AFAICT it does not do any kind of redirect when you load this URL.

Comment 10

[Dale Harvey (:daleharvey)]

RyanVM pointed out that I am actually the triage owner for this component, being that I dont do any other work on mobile and am not given any time to work on this, probably best if someone else is found

Comment 11

[Andreas Bovens [:abovens]]

That's probably a mistake, Dale. Snorp or someone in his team should be able to assist, I believe.

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

I think Dylan is planning to look at this soon.

Comment 13

[Andreas Bovens [:abovens]]

OK, it seems like this is more serious than I thought. All types of PWAs appear to be affected by this, so this is a serious regression: any PWA that I install and launch from the home screen opens in the browser instead of in standalone mode. Stable is not affected. 

Susheel, can this be prioritized?

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

NI sdaswani: note he's on PTO atm.

Snorp, will your team prioritize this and work on it? I want to make sure this isn't forgotten because it sounds like an important regression to fix. If not, please NI me again.

Comment 15

[David Bolter [:davidb] (NeedInfo me for attention)]

Dylan said he'd take a look.

Comment 16

[Dylan Roeh (:droeh) (he/him)]

Jim, I narrowed this down to your patches for bug 1453537, but nothing is immediately jumping out at me. Any ideas/suggestions?

Comment 17

[(inactive) Jim Chen [:jchen] [:darchons]]

The prompt is displayed at [1], so seems like a good start is to debug `onSecurityChange` calls and how its behavior changed.

[1] https://searchfox.org/mozilla-central/rev/11a2ae294f50049e12515b5821f5a396d951aacb/mobile/android/base/java/org/mozilla/gecko/webapps/WebAppActivity.java#117

Comment 18

[:sdaswani]

Clearing NI as it seems to be a platform issues and/or assigned, feel free to re-add me if necessary.

Comment 20

[Dylan Roeh (:droeh) (he/him)]

Attachment: Revert a GV change to fix PWA bustage

It looks like the issue is the removal of browser.stop() from startup() in geckoview.js; without that we (for some reason) get a bogus initial onSecurityChange which causes us to exit the PWA.

Comment 21

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8971227 [details] [diff] [review]
Revert a GV change to fix PWA bustage

Review of attachment 8971227 [details] [diff] [review]:
-----------------------------------------------------------------

We want the initial load for consistency with e10s. We should just ignore the onSecurityChange call for the about:blank load.

Comment 22

[James Willcox (:snorp) ([e-mail]) (he/him)]

I would *really* like to kill the initial about:blank nonsense. Apparently, this is difficult. Bug 1447406 has some stuff on that.

Comment 23

[Dylan Roeh (:droeh) (he/him)]

Attachment: Don't fire onSecurityChange for about:blank

Fair enough, let's just not fire onSecurityChange for that load.

Comment 24

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8971430 [details] [diff] [review]
Don't fire onSecurityChange for about:blank

Review of attachment 8971430 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/modules/geckoview/GeckoViewProgress.jsm
@@ +250,5 @@
>        return;
>      }
>  
> +    // Don't fire onSecurityChanged for about:blank loads.
> +    if (aRequest.QueryInterface(Ci.nsIChannel).URI.displaySpec === "about:blank") {

Wouldn't this disable `onSecurityChange` for all "about:blank" loads? We only want to disable the first one, otherwise if the user explicitly loads "about:blank", they will be left with stale security information.

Comment 25

[(inactive) Jim Chen [:jchen] [:darchons]]

I suggest just putting the workaround in WebAppActivity itself.

Comment 26

[Dylan Roeh (:droeh) (he/him)]

Attachment: Ignore first about:blank load for PWAs.

Comment 27

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8971621 [details] [diff] [review]
Ignore first about:blank load for PWAs.

Review of attachment 8971621 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/java/org/mozilla/gecko/webapps/WebAppActivity.java
@@ +114,5 @@
>  
>              @Override
>              public void onSecurityChange(GeckoSession session, SecurityInformation security) {
> +                // We want to ignore the extraneous first about:blank load
> +                if (mIsFirstLoad) {

Check that |security.origin| starts with "moz-nullprincipal:", which should be true for about:blank

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ebd471b8e07
Ignore onSecurityChange for initial about:blank load in PWAs. r=jchen

Comment 29

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/9ebd471b8e07

Comment 30

[Oana Horvath [:ohorvath - out of office]]

Verified as fixed on Beta 61.0b3.
Devices: 
Sony Xperia Z2 (Android 6.0.1)
Samsung Galaxy Note 8 (Android 7.1.1)