828280 - (packaged) App won't install without `developer` field in its manifest.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Alexandre Poirot [:ochameau]]

We are seeing a installation succeed alert but can see a package installation failure in black bottom overlay. In logcat we can see INVALID_MANIFEST error being fired. We additional debug I found that AppsUtils.compareManifest is broken:
http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/AppsUtils.jsm#332
Following code will throw exception `dev1 is undefined` if we don't specify any `developer` field in the webapp manifest.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 828280 - (packaged) App won't install without `developer` field in its manifest.

Comment 2

[Alexandre Poirot [:ochameau]]

Comment on attachment 699726 [details] [diff] [review]
Bug 828280 - (packaged) App won't install without `developer` field in its manifest.

I've also adding a reportError() instead of debug() in order to figure out issues without having to patch gecko.

Comment 3

[Julien Wajsberg [:julienw]]

Comment on attachment 699726 [details] [diff] [review]
Bug 828280 - (packaged) App won't install without `developer` field in its manifest.

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

::: dom/apps/src/AppsUtils.jsm
@@ +373,5 @@
>      return this._localeProp("launch_path");
>    },
>  
>    get developer() {
> +    return this._localeProp("developer") || { name:"", url: "" };

I think you should do instead:

  return this._localeProp("developer") || {};

Comment 4

[Alexandre Poirot [:ochameau]]

Julien, have you seen gaia bug 815501?
I've set empty strings in order to soften such potential bug in gaia.

Comment 5

[Jason Smith [:jsmith]]

I think this might be a regression from the MANIFEST_MISMATCH patch that was implemented.

Comment 6

[Julien Wajsberg [:julienw]]

Alexandre: if the dev didn't put anything we should not see anything (and not undefined, clearly). if he puts "" that means something else.

But I won't fight for that :)

Comment 7

[Doug Turner (:dougt)]

Will not block on this patch, but will consider a safe patch for uplift.

Comment 8

[Jason Smith [:jsmith]]

(In reply to Doug Turner (:dougt) from comment #7)
> Will not block on this patch, but will consider a safe patch for uplift.

Disagree - this is too common of a case to not block.

Comment 9

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #8)
> (In reply to Doug Turner (:dougt) from comment #7)
> > Will not block on this patch, but will consider a safe patch for uplift.
> 
> Disagree - this is too common of a case to not block.

And we don't have anything enforced on marketplace for this, nor do I think I'll be able to convince the marketplace team on this one (I guarantee I'll get a wont fix and say client has to fix it for v1).

Comment 10

[Doug Turner (:dougt)]

Not blocking per triage, but we will consider a safe patch for uplift.

Comment 11

[Jason Smith [:jsmith]]

(In reply to Doug Turner (:dougt) from comment #10)
> Not blocking per triage, but we will consider a safe patch for uplift.

Still don't agree. I want a rationale why.

It's too basic not to block.

Comment 12

[Jason Smith [:jsmith]]

Yup, I was right. Marketplace doesn't enforce this property. So the argument to not block is wrong.

Comment 13

[Jason Smith [:jsmith]]

This is basically "let's not block for something that's been working for months" that is probably a small patch to fix. The functionality loss is significant too - the app doesn't install.

I really don't see any argument how this could not block.

Comment 14

[Julien Wajsberg [:julienw]]

Please, we lose so much energy doing this back and forth, just request an approval, land this, and let's move forward.

Comment 15

[[:fabrice] Fabrice Desré]

Comment on attachment 699726 [details] [diff] [review]
Bug 828280 - (packaged) App won't install without `developer` field in its manifest.

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

r? pending clarification about my question. Also, please make sure that doesn't break tests.

::: dom/apps/src/AppsUtils.jsm
@@ +373,5 @@
>      return this._localeProp("launch_path");
>    },
>  
>    get developer() {
> +    return this._localeProp("developer") || { name:"", url: "" };

If this is undefined/null, why return something else? Isn't it the case you take care of in AppsUtils.jsm?

Comment 16

[Jason Smith [:jsmith]]

If data is needed from marketplace for the developer property usage, I can get it.

Comment 17

[Jason Smith [:jsmith]]

I'll show up to triage tomorrow to talk about this.

Comment 18

[Matt Basta [:basta]]

The developer field is, per the manifest spec on MDN, optional. If the spec is changed to require the field, I'll update the validator to account for the new requirement. Bear in mind, you'll also nominate more than a few dozen apps for rereview and tick off the editors.

Comment 19

[Jason Smith [:jsmith]]

I actually dug into this. Looks like we don't crash, but we throw:

01-09 17:39:55.984: E/GeckoConsole(815): Content JS INFO at app://system.gaiamobile.org/js/app_install_manager.js:187 in ai_handleDownloadError: downloadError event, error code is INVALID_PACKAGE

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 699726 [details] [diff] [review]
Bug 828280 - (packaged) App won't install without `developer` field in its manifest.

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

::: dom/apps/src/AppsUtils.jsm
@@ +307,5 @@
>          return false;
>        }
>  
> +      return (!dev1 && !dev2) ||
> +             (dev1.name === dev2.name && dev1.url === dev2.url);

This doesn't cover the case when the dev property is available but is empty. Maybe something like:

return ((dev1 && dev1.name) === (dev2 && dev2.name)) &&
       ((dev1 && dev1.url) === (dev2 && dev2.url));


This relies on the lovely feature that in JS |undefined && X| returns undefined.

Comment 21

[Julien Wajsberg [:julienw]]

Comment on attachment 699726 [details] [diff] [review]
Bug 828280 - (packaged) App won't install without `developer` field in its manifest.

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

::: dom/apps/src/AppsUtils.jsm
@@ +307,5 @@
>          return false;
>        }
>  
> +      return (!dev1 && !dev2) ||
> +             (dev1.name === dev2.name && dev1.url === dev2.url);

Are you sure ?

If |dev| is available but empty, then |dev.name| will be undefined, right ? And therefore it will work.

IMHO we cover all the cases : dev1 is undefined but not dev2, dev2 is undefined but not dev1, dev1 and dev2 are both undefined, and dev1 and dev2 are both available.

Comment 22

[Alexandre Poirot [:ochameau]]

Attachment: Bug 828280 - (packaged) App won't install without `developer` field in its manifest.

Comment 23

[Alexandre Poirot [:ochameau]]

Comment on attachment 701075 [details] [diff] [review]
Bug 828280 - (packaged) App won't install without `developer` field in its manifest.

fabrice, I've added a comment about your last review question.
I'm defaulting `developer` to `{}` in parallel of compareManifest fix in order to prevent same kind of exception somewhere else.
Feel free to r- this patch if you consider that it is important to stay in sync with original manifest and avoid any normalization.

Comment 24

[Alexandre Poirot [:ochameau]]

TEST_PATH=dom/tests/mochitest/webapps/ make mochitest-chrome -- PASS!

Comment 25

[Jason Smith [:jsmith]]

Hold up. You need approval on this patch.

Comment 26

[[:fabrice] Fabrice Desré]

That can still land on inbound

Comment 27

[Jason Smith [:jsmith]]

(In reply to Fabrice Desré [:fabrice] from comment #26)
> That can still land on inbound

Yeah that's true. But we should ask for approval on this one.

Comment 28

[Alexandre Poirot [:ochameau]]

Comment on attachment 701075 [details] [diff] [review]
Bug 828280 - (packaged) App won't install without `developer` field in its manifest.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: developers will have hard time figuring out why their app won't install if they omit developer field
Testing completed: app install/update on device and desktop webapps unit tests
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 29

[Hubert Figuiere [:hub]]

Landing 

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea7baeb8f451
https://hg.mozilla.org/releases/mozilla-b2g18/rev/dc4f5e104c4e

(fabrice said to land it)

marking fixed as per the new jst rules

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ea7baeb8f451