1424179 - A2HS badge is shown on site with broken HTTPS (Follow Up)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Toolbar, defect, P1)

Description

[Nevin Chen(Not active on Bugzilla)]

per https://bugzilla.mozilla.org/show_bug.cgi?id=1419245#c22
"
Hi, I've verified your mixed content test page and looks good, the badge doesn't appear.
But, I've noticed that m.aliexpress.com today had the Insecure content warning so I checked and the badge appears on this site and it doesn't do anything when you tap the add to home screen button.
Please have a look at this screen recording: https://drive.google.com/open?id=16vWSaEG-OtV0pK1uSTYGzgaR2h7_xbMu

"

After consulting cosmin_sabou, since bug 1419245 is landed, I'll land the extra fix here.

After this bug is fixed, I'll also want to uplift this bug and bug 1419245

Comment 1

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1424179 - Hide PWA badge when site identity updates to mix content.

https://m.aliexpress.com is a good target to test. But it sometimes show mixed content, sometimes not.
I also chnage the check when adding a PWA shortcut. Only fail fast in nightly and local build.

Review commit: https://reviewboard.mozilla.org/r/206574/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206574/

Comment 2

[Julian Chu [:walkingice]]

Comment on attachment 8935676 [details]
Bug 1424179 - Hide PWA badge when site identity updates to mix content.

https://reviewboard.mozilla.org/r/206574/#review213636

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c97612e2aa6e
Hide PWA badge when site identity updates to mix content. r=walkingice

Comment 4

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/c97612e2aa6e

Comment 5

[Nevin Chen(Not active on Bugzilla)]

HI Oana
Could you please help if this is fixed on Night?
I'll like to request beta uplift if it pases.
see https://bugzilla.mozilla.org/show_bug.cgi?id=1419245#c23

Comment 6

[Nevin Chen(Not active on Bugzilla)]

I'll request for uplift if it pass QA

Comment 7

[Julien Cristau [:jcristau]]

I'm not sure why I'm being needinfo'ed here, but I still don't understand what the intent in this and 1419245 is, and the state of these bugs looks very much confused.

Comment 8

[Oana Horvath [:ohorvath]]

Hi Nevin,
Unfortunately, I can't get to the same behavior on aliexpress or other sites to check the fix. I think the solution would be to have again a test page that covers mixed & insecure content. Also see https://bugzilla.mozilla.org/show_bug.cgi?id=1423587#c1 to make sure we are covered.
Thank you!

Comment 9

[Oana Horvath [:ohorvath]]

I've finally managed to verify the fix today, on aliexpress.com again. So the changes are verified on Nightly 59 (2017-12-20).

Comment 10

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8935676 [details]
Bug 1424179 - Hide PWA badge when site identity updates to mix content.

Approval Request Comment
[Feature/Bug causing the regression]: We had a new feature to let users add PWA websites to users' mobile launcher. We want to add some restrictions here.
[User impact if declined]: Users will be able to add PWA icons with unsafe sites.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: It's done.
[List of other uplifts needed for the feature/fix]:
Bug 1419245 fixed:
If a website is PWA-ready (has manifest) but its certificated is expired, we should hide the PWA badge.
Bug 1424179 fixed:
If a website is PWA-ready (has manifest) but has mixed content, we should hide PWA badge.

[Is the change risky?]: I think it's not risky. 
[Why is the change risky/not risky?]: The worst case is we'll be too strict to the new feature. But this won't break other current features.
[String changes made/needed]: no

Comment 11

[Nevin Chen(Not active on Bugzilla)]

Hi Andreas

There are "Site identity" related scenarios needs to be considered:
1. Before the user adds PWA website to shortcut:
  A. The website doesn't have a valid certificate (e.g. expired or non-https)
  B. The website has mixed content

2. After the user adds PWA website to shortcut, he clicks the PWA shortcut but:
  C. The target url doesn't have a valid certificate (e.g. expired)
  D. The target url has mixed content

Bug 1419245 fixed scenario A
Bug 1424179 fixed scenario B
Bug 1423587 is for C & D, but I think they should be handled by GeckoView (e.g. display a prompt, or fallback to use the main browser app)

As far as I know, Chrome still let you add PWA to shortcut in scenario A,B (tested with https://s3-us-west-2.amazonaws.com/pwa-nevin/mix.html). My question is
1. Do you want to align Chrome's behavior? (That way I'll need to backout Bug 1419245 and Bug 1424179 in Nightly.
or
2. Do you want the fix for A & B (Bug 1419245, Bug 1424179) landed in 58 or 59?

Comment 12

[Andreas Bovens [:abovens]]

Answers to your questions:
1. I think we're doing the right thing here, so it's fine to not align with Chrome's behavior.

2. I'd like to have it in 58.

@snorp, as for bug 1423587 (C & D), when do you think this can be fixed? Will we make it for 58?

Comment 13

[Gerry Chang [:gchang]]

Comment on attachment 8935676 [details]
Bug 1424179 - Hide PWA badge when site identity updates to mix content.

We start to support PWA on 58. Take this to hide the PWA badge when the website has mixed content. Beta58+.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c9992d9c13a8

Comment 15

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

(In reply to Andreas Bovens [:abovens] from comment #12)
I commented in bug 1423587, but I think GeckoView (via GeckoSession) already exposes enough information for the frontend folks to fix this.

Comment 16

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

(In reply to Nevin Chen [:nechen] from comment #11)
> Bug 1423587 is for C & D, but I think they should be handled by GeckoView
> (e.g. display a prompt, or fallback to use the main browser app)

GeckoView in general does not display prompts, but delegates these tasks to the app. In the same vein, it does not launch other apps for fallback scenarios, etc.

Comment 17

[Oana Horvath [:ohorvath]]

Verified the fix on Beta 58.0b15.
Device: HTC Desire 820 (Android 6.0.1)