1347154 - Cannot install twitter as a PWA

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Comment 1

[Dale Harvey (:daleharvey)]

We are failing to fetch the icon on twitter when we attempt to install (there is also a small bug around ths icon fetching fallback code)

If I comment out

  //request.overrideContentPolicyType(Ci.nsIContentPolicy.TYPE_WEB_MANIFEST);

From the icon fetching code it works, it seems like the contentPolicy is saying it only wants to accept certain type of resources (I think I originally assumed it mean this was a manifest originating request)

Comment 2

[Dale Harvey (:daleharvey)]

Hrm, request.overrideContentPolicyType(Ci.nsIContentPolicy.TYPE_IMAGE); doesnt seem to work here either, its worth noting it is a cross origin request, this doesnt seem to happen when fetching other PWA's icons (which are mostly same origin)

https://ma-0.twimg.com/twitter-assets/responsive-web/web/ltr/icon-ios.a9cd885bccbcaf2f.png when installing https://mobile.twitter.com/home

Marcos any idea what is causing this and what is the harm of removing that line? twitter is most definitely a pwa we need to work

Comment 3

[Dale Harvey (:daleharvey)]

Attachment: Set correct csp for icon fetching

Actually my test was premature, if I set TYPE_IMAGE then it fetches the icon successfully, also clarified the variable name

Comment 4

[Marcos Caceres [:marcosc]]

Comment on attachment 8848044 [details] [diff] [review]
Set correct csp for icon fetching

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

Sorry for the delay! Couple of things to consider, but generally all good :)

::: dom/manifest/ManifestIcons.jsm
@@ +68,5 @@
>    });
>  }
>  
>  function fetchIcon(aWindow, src) {
> +  const iconURL = new aWindow.URL(src);

If the URL might be relative, maybe always add a base? 

```
new aWindow.URL(src, base); // but not sure what to use as base (aWindow.location?)
```

Also, beware that "new aWindow.URL(src);" can throw, so you might want to:

```
let iconURL;
try{
  iconURL new aWindow.URL(src);
} catch(err){
   return Promise.reject(err);
}
```

Alternatively, change fetchIcon() itself to be an async function?

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c14b29f1788
Set correct csp for icon fetching. r=marcosc

Comment 6

[Dale Harvey (:daleharvey)]

Thats great

> Also, beware that "new aWindow.URL(src);" can throw, so you might want to

The |.catch| in the the caller will catch that whether fetchIcon is async or not right, no need for a try catch? either way since its returning a promise no harm and a little clearer to make it async

Add the base location on the way in too and landed, thanks

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8c14b29f1788

Comment 8

[Oana Horvath [:ohorvath]]

Verified as fixed on Nightly 55 (2017-03-31).
Devices:
-HTC Nexus 9 (Android 7.1.1)
-Huawei Honor 8 (Android 6.0)
-LG G4 (Android 5.1)