Closed Bug 1347154 Opened 7 years ago Closed 7 years ago

Cannot install twitter as a PWA

Categories

(Firefox for Android Graveyard :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox55 verified)

VERIFIED FIXED
Firefox 55
Tracking Status
firefox55 --- verified

People

(Reporter: daleharvey, Assigned: daleharvey)

References

Details

Attachments

(1 file)

      No description provided.
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)
Assignee: nobody → dale
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
Flags: needinfo?(mcaceres)
Blocks: 1285858
Actually my test was premature, if I set TYPE_IMAGE then it fetches the icon successfully, also clarified the variable name
Flags: needinfo?(mcaceres)
Attachment #8848044 - Flags: review?(mcaceres)
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?
Attachment #8848044 - Flags: review?(mcaceres) → review+
Pushed by dharvey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c14b29f1788
Set correct csp for icon fetching. r=marcosc
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
https://hg.mozilla.org/mozilla-central/rev/8c14b29f1788
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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)
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: