If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Give clearer/cleaner errors when validate_resource_hostname() throws.

RESOLVED FIXED

Status

Add-on SDK
General
P3
normal
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: Gomita, Assigned: KWierso)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv:1.9.3a5pre) Gecko/20100426 Minefield/3.7a5pre
Build Identifier: 

The XPI file exported by 'cfx xpi' command is named after the value of the "name" property in package.json.
If the value of the "name" property contains some invalid characters (\ / * : ? " < > |) which we cannot use as a part of file name, 'cfx xpi' fails to export XPI.

Reproducible: Always

Steps to Reproduce:
1. set the value of the "name" property in package.json as "Hello * World".
2. try to export XPI by 'cfx xpi' command.
Actual Results:  
Fails to export XPI file with the following error message.
IOError: [Errno 22] invalid mode ('wb') or filename: u'Hello * World.xpi'

Expected Results:  
Succcessfully exports XPI file.
(Reporter)

Comment 1

8 years ago
I think the XPI file name should be named after the package directory name like: 'hello-world.xpi' (assuming that the name of the package root directory is 'hello-world').

Comment 2

8 years ago
Oh, so theoretically long-ish names with special characters is what the 'fullName' property in the package.json is for:

  https://jetpack.mozillalabs.com/sdk/0.2/docs/#guide/package-spec

'name' is actually not meant to be something with those funky characters, and it actually defaults to the name of the package's directory if it's not in package.json--though this may conflict with the CommonJS package specification, I'm not sure.

In any case, thanks for the report--we should definitely not raise a silly IOError in this instance, at the very least!
(Reporter)

Comment 3

8 years ago
Sorry, I missed to read the spec of package.json.
Now I understand that I should use "fullName" property instead of "name" to display a funcy name in the Extensions Manager.
Should I change the status of the bug to "RESOLVED"?

I found another related bug.
If the value of "name" property in package.json and the root directory name both contain a white-space such like "hello world",
'cfx run' command fails with the following error message:

(C:\jetpack-sdk-0.3) C:\jetpack-sdk-0.3\packages\hello world>cfx run -a firefox
[Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIXPCComponents_Utils.import]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: file:///c:
/docume~1/xxx/locals~1/temp/xxx.mozrunner/extensions/xulapp@toolness.com/components/harness.js :: buildLoader :: line 163"  data: no] (undefined:163)
FAIL

Should I file this as a separated bug?
(Reporter)

Comment 4

8 years ago
Sorry again, the package spec says that: "This name cannot contain spaces."

Comment 5

8 years ago
No, it's not your fault! We're providing terrible error feedback here--we should detect for these kinds of cases in cfx and give more helpful warnings, like:

"Sorry, but thee 'name' property of packages/foo/package.json cannot be 'foo bar' because it contains illegal characters. You may want to use the 'fullName' property instead. For more information, see the package.json documentation at static-files/md/dev-guide/package-spec.md."

How does something like that sound?
The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".
Component: Jetpack SDK → General
Product: Mozilla Labs → Add-on SDK
QA Contact: jetpack-sdk → general
It sounds like we should:

* validate that the name field only contains valid characters
* strip out invalid characters when creating a XPI
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Priority: -- → P3
Hardware: x86 → All
Target Milestone: --- → Future
(Assignee)

Comment 8

6 years ago
Is this still happening?
Whiteboard: [triage:followup]
(Assignee)

Comment 9

6 years ago
(Pushing all open bugs to the --- milestone for the new triage system)
Target Milestone: Future → ---
(Assignee)

Comment 10

6 years ago
Myk: I never got confirmation that this was still happening after setting the triage flag a month ago. Do you know if we already do the things you listed in comment 7?
Yes, this is still happening.  To test this, I created an addon using "cfx init", changed its name in package.json to "foo/bar", and called "cfx xpi" to try to package it, whereupon cfx printed the following error with traceback:

Traceback (most recent call last):
  File "c:/Users/myk/Dropbox/addon-sdk/bin/cfx", line 29, in <module>
    cuddlefish.run()
  File "c:\Users\myk\Dropbox\addon-sdk\python-lib\cuddlefish\__init__.py", line 637, in run
    include_dep_tests=options.dep_tests
  File "c:\Users\myk\Dropbox\addon-sdk\python-lib\cuddlefish\packaging.py", line 322, in generate_build_for_target
    add_section_to_build(target_cfg, "tests", is_code=True)
  File "c:\Users\myk\Dropbox\addon-sdk\python-lib\cuddlefish\packaging.py", line 297, in add_section_to_build
    validate_resource_hostname(name)
  File "c:\Users\myk\Dropbox\addon-sdk\python-lib\cuddlefish\packaging.py", line 85, in validate_resource_hostname
    raise ValueError('invalid resource hostname: %s' % name)
ValueError: invalid resource hostname: jid1-qwouchluzbmukq-at-jetpack-foo/bar-tests

cfx should instead tell me without traceback that my addon's name contains invalid characters, if indeed "/" is an invalid character in addon names.  Otherwise, it should strip out that character when using the addon name as a resource hostname, XPI filename, etc.

(And of course it should do the same for other such characters.)
(Assignee)

Comment 12

6 years ago
(In reply to Myk Melez [:myk] from comment #11)
So basically, in https://github.com/mozilla/addon-sdk/blob/3eb964d2c25963a0516c59cd072a08b33ab29013/python-lib/cuddlefish/packaging.py#L76 it should catch the errors raised in those three ifs? (Or just not raise any errors but just modify 'name'?)
Whiteboard: [triage:followup]
(In reply to Wes Kocher (:KWierso) (Jetpack Bugmaster) from comment #12)
> (In reply to Myk Melez [:myk] from comment #11)
> So basically, in
> https://github.com/mozilla/addon-sdk/blob/
> 3eb964d2c25963a0516c59cd072a08b33ab29013/python-lib/cuddlefish/packaging.
> py#L76 it should catch the errors raised in those three ifs? (Or just not
> raise any errors but just modify 'name'?)

Hmm, yes, I think that's right.
(Assignee)

Comment 14

6 years ago
Created attachment 577802 [details]
Pointer to pull request 278

So this pull request just changes the wording of the error being raised when invalid stuff is detected. It now mentions that the name must be only letters, numbers, dashes and underscores, instead of just "invalid resource hostname".


Also, as of some recent commit, it no longer shows the jid-style URI in the error, it just shows the package name's value.

The pull request doesn't strip out invalid characters or hide the traceback, but it's at least an improvement over what was there.
Attachment #577802 - Flags: review?(myk)
(Assignee)

Updated

6 years ago
Assignee: nobody → kwierso
Comment on attachment 577802 [details]
Pointer to pull request 278

Comments in pull request.
Attachment #577802 - Flags: review?(myk) → review-
(Assignee)

Comment 16

6 years ago
Comment on attachment 577802 [details]
Pointer to pull request 278

Ready for another review, Myk.
Attachment #577802 - Flags: review- → review?(myk)
(Assignee)

Comment 17

6 years ago
Comment on attachment 577802 [details]
Pointer to pull request 278

Passing review request to markh.
Attachment #577802 - Flags: review?(myk) → review?(mhammond)
Comment on attachment 577802 [details]
Pointer to pull request 278

looks good!
Attachment #577802 - Flags: review?(mhammond) → review+
(Assignee)

Comment 19

6 years ago
Renaming bug to what the patch actually does.
Summary: XPI exported by 'cfx xpi' should not be named after "name" property in package.json → Give clearer/cleaner errors when validate_resource_hostname() throws.

Comment 20

6 years ago
Commit pushed to https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/95f03637db95a53c1169804e19e54e9cdb43f7b4
Bug 561944 - Give clearer/cleaner errors when validate_resource_hostname() throws.  r=@mhammond

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.