999399 - [App Manager] The app manager should emit a warning when a manifest has '/' as launch_path for a packaged app

Status: RESOLVED WONTFIX

(DevTools Graveyard :: WebIDE, defect, P3)

Description

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

STR:
* use a manifest.webapp with launch_path: '/'
* try to package the app

Expected:
* we should have a warning because such an app is unusable: launching it on the device would show the content of the package instead of the index

Actual:
* no error

Comment 1

[Alexandre Poirot [:ochameau] [PTO back August 21]]

... also, the launch_path is mandatory for packaged apps.
If you don't set it, you will also see the directory listing.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

If this is just a bunch of string comparisons then perhaps it would be a [good first bug]?

Comment 3

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Yep, it should only be about hacking this method:
  http://mxr.mozilla.org/mozilla-central/source/browser/devtools/app-manager/app-validator.js#133

Comment 4

[vikneshwar]

Attachment: added-path-test

Comment 5

[Paul Rouget [:paul]]

This will need to wait until we land the new UI.

Comment 6

[Paul Rouget [:paul]]

Sorry, didn't mean to comment here.

Comment 7

[Paul Rouget [:paul]]

Comment on attachment 8420269 [details] [diff] [review]
added-path-test

I think we should no add an error, but a warning. The app is still valid and can be installed. Apps with entry points don't need a direct launch_path. Example: https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/manifest.webapp

1. check if path is "/" (no need to check " " because of the if statement before)
2. if so, is there an "entry_points" property?
3. Yes? It's valid.
4. No? It's valid too, but add a warning: "Entry point is '/'. Expecting a file or entry points."

You'll need to add a new string in app-manager.properties.

Also, I see in app-validator.js:

> if (this.project.type == "packaged") {
>    path = "." + ( manifest.launch_path || "/index.html" );

That will make the path valid if if the property doesn't exist. We might want to check the presence of this property earlier in the function.

Also, iirc, we want to drop entry_points at some point, so maybe we don't want to mention it or support it.

Alex, what do you think?

Comment 8

[Pablo Bernardo (voylinux)]

Hello, if this is good for a first bug, I would like to work on it.
I have been said in IRC that those bugs have a mentor who can help to begin.

Comment 9

[Alexandre Poirot [:ochameau] [PTO back August 21]]

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #7)
> Comment on attachment 8420269 [details] [diff] [review]
> added-path-test
> 
> I think we should no add an error, but a warning. The app is still valid and
> can be installed. Apps with entry points don't need a direct launch_path.
> Example:
> https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/manifest.
> webapp

We are already dispatching error for any error around launch_path,
so it sounds like we should either keep notifying errors or switch them to warnings.
But to be honest, even if the app launches with a bad launch path,
we can consider ir as an error as the app won't be much functional regarding end users!

> 
> 1. check if path is "/" (no need to check " " because of the if statement
> before)
> 2. if so, is there an "entry_points" property?
> 3. Yes? It's valid.
> 4. No? It's valid too, but add a warning: "Entry point is '/'. Expecting a
> file or entry points."

The fact that entry_points are declared doesn't mean we can't launch the app without entry_points,
so we should validate root launch_path anyway.
If you are interested, you may add some new code in app-validator.js to also check entry_points's launch_path by tweaking and reusing validateLaunchPath function.

> 
> You'll need to add a new string in app-manager.properties.

+1

> 
> Also, I see in app-validator.js:
> 
> > if (this.project.type == "packaged") {
> >    path = "." + ( manifest.launch_path || "/index.html" );
> 
> That will make the path valid if if the property doesn't exist. We might
> want to check the presence of this property earlier in the function.

At the end, I think the current code of validateManifest is designed for hosted apps,
and very poorly check packaged apps constraints.
It may be easier to have distinct checks between packaged and hosted.
Instead of resolving launch_path to a URL, we may resolve it to a file for packaged apps:
if (is packaged) {
  let path = "." + ( manifest.launch_path || "/index.html" );
  let uri = Services.io.newURI(path, null, Services.io.newURI(origin, null, null));
  let file = uri.QueryInterface(Ci.nsIFileURL).file;
  if (!file.exists() || !file.isFile()) {
    this.error(...the launch path doesn't map to any valid file...);
  }
} else {
  // current validate manifest code
}
> 
> Also, iirc, we want to drop entry_points at some point, so maybe we don't
> want to mention it or support it.

We aren't going away from entry points any time soon, but yes, there has been some words about stop using entry points in gaia apps...

Comment 10

[Mike Hoye [:mhoye]]

Hi, Pablo - it looks like this bug is already being worked on, but if you search bugzilla for [good first bug] there are plenty to choose from.

Comment 12

[Hamilton Chevez]

Hi, I would be interested in making this my first bug to work on. Not sure how to proceed after this though, do I just go and start working on it or is there a guide I should have read somewhere? Sorry for any inconvenience, just point me in the right direction.

Comment 13

[Hamilton Chevez]

Hi, I would be interested in making this my first bug to work on. Not sure how to proceed after this though, do I just go and start working on it or is there a guide I should have read somewhere? Sorry for any inconvenience, just point me in the right direction.

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Hamilton Chevez from comment #13)
> Hi, I would be interested in making this my first bug to work on. Not sure
> how to proceed after this though, do I just go and start working on it or is
> there a guide I should have read somewhere? Sorry for any inconvenience,
> just point me in the right direction.

Hopefully Alex will reply soon!  In any case, I would say take a look at [1] (for Git users) and [2] (for Mercurial users).

The module related to this bug lives at https://dxr.mozilla.org/mozilla-central/source/devtools/client/webide/modules/app-validator.js.

[1]: https://developer.mozilla.org/en-US/docs/Tools/Contributing
[2]: https://wiki.mozilla.org/DevTools/Hacking

Comment 15

[Alexandre Poirot [:ochameau] [PTO back August 21]]

App support is at risk, so I'm not sure there is a lot of value in fixing this.
The future of packaged apps in gecko is linked to Firefox OS, which is about to stop using packaged apps and instead use http.

But the links posted in comment 14 would help fixing any other devtools bug.

Comment 16

[Patrick Brosset <:pbro>]

[good first bug] whiteboard -> keyword mass change

Comment 17

[Key]

I'd like to be assigned to this bug, it would be my first.

Comment 18

[Mike Hoye [:mhoye]]

Give it a try; please feel free to use the needinfo checkbox below if you've got any questions.

Thank you!

Comment 19

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I don't believe this bug makes much sense anymore, because apps have gone away with B2G and WebIDE is also planned for removal.

I'd suggest checking http://firefox-dev.tools for other bugs!