967735 - WebappManager throws "TypeError: aManifest.orientation.join is not a function" on non-array value

Status: RESOLVED FIXED

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

Description

[Myk Melez [:myk] [@mykmelez]]

WebappManager assumes that the manifest's orientation property is an array:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/WebappManager.jsm?rev=efcbcdb857e2#231

As specified by the Manifest specification:

https://developer.mozilla.org/en-US/Apps/Developing/Manifest#orientation

But Marketplace sometimes serves apps whose manifests have string orientation values, like the manifest for Cut The Rope, whose "orientation" value is set to the string "landscape":

{
  "version": "1.3",
  "name": "CutTheRope",
  "description": "Cut the Rope, catch a star, and feed Om Nom candy in this award-winning game!",
  "icons": {
    "16": "/favicon.png",
    "64": "/fb64.png",
    "128": "/fb128.png",
    "256": "/fb.png"
  },
  "developer": {
    "name": "ZeptoLab",
    "url": "http://www.zeptolab.com"
  },
  "launch_path": "/index.html",
  "orientation": "landscape",
  "fullscreen": "true",
  "permissions": {
    "audio-channel-content": {
      "description": "Playing audio"
    }
  }
}

Which causes WebappManager to throw an exception:

*************************
A coding exception was thrown in a Promise resolution callback.
Full message: TypeError: aManifest.orientation.join is not a function
Full stack: this.WebappManager.writeDefaultPrefs@resource://gre/modules/WebappManager.jsm:231
this.WebappManager.askInstall/</<@resource://gre/modules/WebappManager.jsm:133
this.DOMApplicationRegistry._onDownloadPackage/<@resource://gre/modules/Webapps.jsm:2377
Handler.prototype.process@resource://gre/modules/Promise.jsm:767
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm:531
*************************

Because even though WebappManager checks if the value is defined before trying to access it, it doesn't check if it's actually an array.

So WebappManager should not assume that the value of the orientation property is valid and gracefully handle an invalid value.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Per discussions with folks, it sounds like other runtimes are being lenient and allowing a string value for this field.  So if it's a string, then let's pretend it's an array with that string as its only item.

Comment 2

[Martyn Haigh (:mhaigh)]

Attachment: Check orientation type and convert appropiately if incorrect

Check object type of orientation and convert from string to array if necessary

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8372396 [details] [diff] [review]
Check orientation type and convert appropiately if incorrect

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

Looks good to me!

::: mobile/android/modules/WebappManager.jsm
@@ +231,5 @@
> +        let orientation = aManifest.orientation;
> +        if (typeof aManifest.orientation === 'string') {
> +          orientation = [ aManifest.orientation ];
> +        }
> +        prefs.push({name:"app.orientation.default", value: orientation.join(",") });

Nit: add a space between "{" and "name:" and between "name:" and "app.orientation.default".

Comment 4

[Martyn Haigh (:mhaigh)]

Attachment: Check orientation type and convert appropiately if incorrect

Fixed nits

Comment 5

[Wesley Johnston (:wesj)]

Comment on attachment 8373245 [details] [diff] [review]
Check orientation type and convert appropiately if incorrect

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

Little change. No need to re-review if you don't want to.

::: mobile/android/modules/WebappManager.jsm
@@ +228,5 @@
>        // build any app specific default prefs
>        let prefs = [];
>        if (aManifest.orientation) {
> +        let orientation = aManifest.orientation;
> +        if (typeof aManifest.orientation === 'string') {

I have a dumb personal preference for Array.isArray(). In this case it is kinda nicer though (i.e. we're not creating a temp array that we just toss. Can we use it instead? i.e.

if (Array.isArray(orientation)) {
  orientation = orientation.join(",");
}
prefs.push({});

Comment 6

[Martyn Haigh (:mhaigh)]

Attachment: Check orientation type and convert appropiately if incorrect

refactored code based on feedback

Comment 7

[Myk Melez [:myk] [@mykmelez]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/58ce2022d43b

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/58ce2022d43b

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8374120 [details] [diff] [review]
Check orientation type and convert appropiately if incorrect

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Bug 934756.
User impact if declined: 
  Certain apps will launch in the wrong orientation (portrait vs. landscape).
Testing completed (on m-c, etc.): 
  This has been on Central for two days.
Risk to taking this patch (and alternatives if risky): 
  This is a low-risk patch with an obvious fix that improves validation of data
  that might be of an unexpected type.
String or IDL/UUID changes made by this patch:
  None.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1b5e18e67fdf