Closed Bug 967735 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: Web Apps, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 + fixed
firefox30 --- fixed

People

(Reporter: myk, Assigned: mhaigh)

Details

Attachments

(1 file, 2 obsolete files)

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.
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.
Assignee: nobody → mhaigh
Check object type of orientation and convert from string to array if necessary
Attachment #8372396 - Flags: feedback?(myk)
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".
Attachment #8372396 - Flags: review?(wjohnston)
Attachment #8372396 - Flags: feedback?(myk)
Attachment #8372396 - Flags: feedback+
Fixed nits
Attachment #8372396 - Attachment is obsolete: true
Attachment #8372396 - Flags: review?(wjohnston)
Attachment #8373245 - Flags: review?(wjohnston)
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({});
Attachment #8373245 - Flags: review?(wjohnston) → review+
refactored code based on feedback
Attachment #8373245 - Attachment is obsolete: true
Attachment #8374120 - Flags: review?(wjohnston)
Attachment #8374120 - Flags: review?(wjohnston) → review+
https://hg.mozilla.org/mozilla-central/rev/58ce2022d43b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
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.
Attachment #8374120 - Flags: approval-mozilla-aurora?
Attachment #8374120 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.