Cannot install twitter as a PWA

VERIFIED FIXED in Firefox 55

Status

()

VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: daleharvey, Assigned: daleharvey)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox55 verified)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Comment 1

2 years ago
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
(Assignee)

Comment 2

2 years ago
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)
(Assignee)

Updated

2 years ago
Blocks: 1285858
(Assignee)

Comment 3

2 years ago
Created attachment 8848044 [details] [diff] [review]
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
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+

Comment 5

2 years ago
Pushed by dharvey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c14b29f1788
Set correct csp for icon fetching. r=marcosc
(Assignee)

Comment 6

2 years ago
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8c14b29f1788
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
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-firefox55: fixed → verified
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.