862418 - make the build more strict with bad metadata.json files

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Julien Wajsberg [:julienw] (PTO -> Aug 24)]

We found that in some partner customizatons (eg [1]) the metadata.json files are wrong.

I'd like that we make the build stricter now so that we don't have strange builds later:

- make the manifestURL property mandatory, and fail if this is missing. (instead of having a magic concatenation)
- check that the origin is a real origin (ie: no path at the end). Investigate if we can infer this origin from the manifestURL.
- issue a warning when the manifestURL is an "app://" url, because this makes the packaged app unupdatable (which is sometimes desirable).

[1] https://github.com/telefonicaid/firefoxos-gaia-spain

Asking tef+ on this because while it's npotb I want that our partners are aware of this, and I want to prioritize this.

Comment 1

[Jason Smith [:jsmith]]

FWIW, I think this is a good idea to do for defect prevention, but I don't think a makefile protection in itself should block the release. But I definitely suggest going forward implementing this.

Comment 2

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

thank you Julien for telling me about origin incorrect things. I also filed bugs on telefonica repositories and gaia-preload-app for incorrect origin.

https://github.com/yurenju/gaia-preload-app/issues/5
https://github.com/telefonicaid/firefoxos-gaia-spain/issues/2
https://github.com/telefonicaid/firefoxos-gaia-latam/issues/2

Comment 3

[Johnny Stenback (:jst)]

We won't block on this, but we'll track this so we get to this in future releases.

Comment 4

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Fixed it on gaia-preload-app and I will make new version for telefonica repository on bug 864617.

Comment 5

[Julien Wajsberg [:julienw] (PTO -> Aug 24)]

another check we need to do :

* check that we have a manifest.webapp

Comment 6

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

should we check etag in metadata.json?

Comment 7

[[:fabrice] Fabrice Desré]

(In reply to Yuren Ju [:yurenju] from comment #6)
> should we check etag in metadata.json?

I don't think. We don't want to do network access at build time.

Comment 8

[Julien Wajsberg [:julienw] (PTO -> Aug 24)]

Attachment: patch v1

* be stricter about metadata.json data
* fix the apps we have in the tree
---
 build/webapp-manifests.js                     |   91 +++++++++++++++++++------
 test_external_apps/hoststubtest/metadata.json |    2 +-
 test_external_apps/mochitest/metadata.json    |    3 +-
 3 files changed, 75 insertions(+), 21 deletions(-)

Comment 9

[[:fabrice] Fabrice Desré]

Comment on attachment 750509 [details] [diff] [review]
patch v1

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

Please use nsIURI for all the uri checks instead of string based manipulation.

Comment 10

[Julien Wajsberg [:julienw] (PTO -> Aug 24)]

Attachment: patch v2

* be stricter about metadata.json data
* fix the apps we have in the tree
---
 build/webapp-manifests.js                     |   95 ++++++++++++++++++++-----
 test_external_apps/hoststubtest/metadata.json |    2 +-
 test_external_apps/mochitest/metadata.json    |    3 +-
 3 files changed, 79 insertions(+), 21 deletions(-)

Now uses nsIURI, thanks for the great suggestion.

Comment 11

[[:fabrice] Fabrice Desré]

Comment on attachment 751059 [details] [diff] [review]
patch v2

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

r=me with nits fixed.

::: build/webapp-manifests.js
@@ +4,5 @@
>    //dump('-*- webapp-manifest.js: ' + msg + '\n');
>  }
>  
> +let io = Components.classes['@mozilla.org/network/io-service;1']
> +                .getService(Components.interfaces.nsIIOService);

nit: s/Components.classes/Cc, and align .getService with ['@mozilla.org/network...

@@ +135,5 @@
> +                'mandatory manifestURL property.');
> +    return;
> +  }
> +
> +  let manifestURLURI;

very weird variable name. What about manifestURI ?

Comment 12

[[:fabrice] Fabrice Desré]

Comment on attachment 751059 [details] [diff] [review]
patch v2

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

r=me with nits fixed.

::: build/webapp-manifests.js
@@ +4,5 @@
>    //dump('-*- webapp-manifest.js: ' + msg + '\n');
>  }
>  
> +let io = Components.classes['@mozilla.org/network/io-service;1']
> +                .getService(Components.interfaces.nsIIOService);

nit: s/Components.classes/Cc, and align .getService with ['@mozilla.org/network...

@@ +135,5 @@
> +                'mandatory manifestURL property.');
> +    return;
> +  }
> +
> +  let manifestURLURI;

very weird variable name. What about manifestURI ?

Comment 13

[Julien Wajsberg [:julienw] (PTO -> Aug 24)]

Attachment: patch v3

fixed nits, carrying r=fabrice
PR in https://github.com/mozilla-b2g/gaia/pull/9862 to see if Travis is happy

Comment 14

[Julien Wajsberg [:julienw] (PTO -> Aug 24)]

master: 7d8f462d9402c306fa243c96c3d660a399128acc

Comment 15

[Julien Wajsberg [:julienw] (PTO -> Aug 24)]

a=npotb
v1-train: 9380ceb81b3eac45861b8d1be07ab7f748ed52a3
v1.0.1: 62a6ed13e3e135e7183a1af2eef3b55f5ffb0378