Closed Bug 828280 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

defect
Not set
normal

Tracking

(blocking-basecamp:-, firefox20 wontfix, firefox21 fixed, b2g18+ fixed)

RESOLVED FIXED
B2G C4 (2jan on)
blocking-basecamp -
Tracking Status
firefox20 --- wontfix
firefox21 --- fixed
b2g18 + fixed

People

(Reporter: ochameau, Assigned: ochameau)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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 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.
Attachment #699726 - Flags: review?(fabrice)
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") || {};
blocking-basecamp: --- → ?
Julien, have you seen gaia bug 815501?
I've set empty strings in order to soften such potential bug in gaia.
blocking-basecamp: ? → ---
blocking-basecamp: --- → ?
I think this might be a regression from the MANIFEST_MISMATCH patch that was implemented.
Keywords: regression
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 :)
Will not block on this patch, but will consider a safe patch for uplift.
blocking-basecamp: ? → -
(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.
blocking-basecamp: - → ?
(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).
Not blocking per triage, but we will consider a safe patch for uplift.
Assignee: nobody → poirot.alex
blocking-basecamp: ? → -
(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.
blocking-basecamp: - → ?
Yup, I was right. Marketplace doesn't enforce this property. So the argument to not block is wrong.
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.
Please, we lose so much energy doing this back and forth, just request an approval, land this, and let's move forward.
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?
Attachment #699726 - Flags: review?(fabrice)
If data is needed from marketplace for the developer property usage, I can get it.
I'll show up to triage tomorrow to talk about this.
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.
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
blocking-basecamp: ? → -
tracking-b2g18: --- → +
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 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 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.
Attachment #701075 - Flags: review?(fabrice)
TEST_PATH=dom/tests/mochitest/webapps/ make mochitest-chrome -- PASS!
Attachment #699726 - Attachment is obsolete: true
Attachment #701075 - Flags: review?(fabrice) → review+
Keywords: checkin-needed
Hold up. You need approval on this patch.
Keywords: checkin-needed
That can still land on inbound
(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.
Keywords: checkin-needed
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:
Attachment #701075 - Flags: approval-mozilla-b2g18?
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
Status: NEW → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Attachment #701075 - Flags: approval-mozilla-b2g18?
Keywords: verifyme
QA Contact: jsmith
Keywords: verifyme
Target Milestone: --- → B2G C4 (2jan on)
You need to log in before you can comment on or make changes to this bug.