566812 - "Bad ID string" when installing a rebootless-generated-xpi

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Dietrich Ayala (:dietrich)]

[Exception... "Bad ID string"  nsresult: "0x80570031 (NS_ERROR_XPC_BAD_ID_STRING)"  location: "JS frame :: file:///home/dietrich/bin/firefox/modules/XPIProvider.jsm -> file:///home/dietrich/.mozilla/firefox/f9s501ot.widge/extensions/jid0-8O4s3e05BbNHwNUDpsDIOhQcF5w@jetpack/bootstrap.js -> file:///home/dietrich/.mozilla/firefox/f9s501ot.widge/extensions/jid0-8O4s3e05BbNHwNUDpsDIOhQcF5w@jetpack/components/harness.js :: anonymous :: line 241"  data: no] (undefined:241)
[Exception... "Bad ID string"  nsresult: "0x80570031 (NS_ERROR_XPC_BAD_ID_STRING)"  location: "JS frame :: file:///home/dietrich/bin/firefox/modules/XPIProvider.jsm -> file:///home/dietrich/.mozilla/firefox/f9s501ot.widge/extensions/jid0-8O4s3e05BbNHwNUDpsDIOhQcF5w@jetpack/bootstrap.js -> file:///home/dietrich/.mozilla/firefox/f9s501ot.widge/extensions/jid0-8O4s3e05BbNHwNUDpsDIOhQcF5w@jetpack/components/harness.js :: anonymous :: line 241"  data: no] (undefined:241)

Comment 1

[Dietrich Ayala (:dietrich)]

this is with an xpi using the new widget module, against trunk with a new profile.

Comment 2

[Dietrich Ayala (:dietrich)]

after restarting, i get the same error once, so something got stuck. the addon doesn't show up in about:addons.

Comment 3

[Dietrich Ayala (:dietrich)]

confirmed same problem with a hello world xpi.

Comment 4

[Dave Townsend [:mossop]]

Looks like the bootstrap.classID property in harness-options.json is a bad GUID. Can you attach an example?

Comment 5

[Dietrich Ayala (:dietrich)]

Attachment: harness-options.json

Comment 6

[Dave Townsend [:mossop]]

The code is trying to use "jid0-8O4s3e05BbNHwNUDpsDIOhQcF5w@jetpack" as a class ID for an XPCOM component, but the class IDs of XPCOM components must be of a GUID form. I would guess that bug 553020 regressed this.

Comment 7

[Brian Warner [:warner :bwarner]]

Yeah, so I guess there are different requirements for these class IDs than for extension IDs in general. I'm using the same jetpackID for several things (all around cuddlefish/__init__.py line 384. As Mossop pointed out on IRC, we can probably generate a unique random classID each time the XPI is created, unrelated to the jetpackID.

Comment 8

[Brian Warner [:warner :bwarner]]

Attachment: set contract/classID to a GUID, not the JID

here's a preliminary patch, to set the contract and class IDs to a GUID that's regenerated each time you build the XPI. The jetpack JID is used as the add-on ID (in the RDF file) and in the harness-options.json file to be available to add-on code via packaging.jetpackID .

It looks like this changes the bundled resources to have pathnames identified with the GUID, not the JID, and I think that is likely to cause problems.

Comment 9

[Brian Warner [:warner :bwarner]]

Attachment: set contract/classID to a GUID, not the JID

updated to set bundled-resource URLs from the JID, not the GUID

Comment 10

[Dave Townsend [:mossop]]

I don't think you want to randomise the contractID here, it should match the add-on ID or extension code won't be able to access it sanely. Only the classID needs to be GUID form.

Comment 11

[Atul Varma [:atul]]

Aye, if the contractID is randomized then the instructions outlined in the "Using the SDK with XUL extensions" appendix of the SDK docs won't work:

  https://jetpack.mozillalabs.com/sdk/0.3/docs/#guide/xul-extensions

Comment 12

[Brian Warner [:warner :bwarner]]

Attachment: set classID to a GUID, not the JID

this version leaves the ContractID=JID, and only sets the ClassID to a randomly-generated GUID. Resource URLs are based upon the JID, and at least a basic test seems to work.

Comment 13

[Atul Varma [:atul]]

One of the problems seems to be that standard, normalized URLs are supposed to have all-lowercase characters in their hostnames. nsStandardURL::BuildNormalizedSpec() seems to do the lowercasing:

  http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsStandardURL.cpp#569

In resource URIs of the form resource://abc/x/y/z, 'abc' is the hostname and presumably has the same constraints as a hostname in a standard URL. So I guess this means it should be all lowercase.

Based on trial-and-error, the "@" character also shouldn't be in hostnames, possibly because it gets interpreted as the username part of a URL or something.

Removing/replacing "@" characters and lowercasing the jetpack id appears to solve the problem, though I'm not sure if there's any other constraints for standard URLs that might be run into as edge-cases if we put some transformation of the jid into the resource URLs.

Comment 14

[Atul Varma [:atul]]

Also, as mentioned on IRC, we should probably have the cuddlefish.preflight logic run for 'cfx run/test' so that it can simulate conditions as close to those of running a real XPI as possible--perhaps w/ the forgiveness that if 'id' isn't present in package.json, it falls back to 'name' instead of creating a keypair, modifying package.json (which may be tracked by hg) and requiring the user to re-run cfx.

Comment 15

[Atul Varma [:atul]]

In the spirit of 'fail early, fail often' I've added a validation step in cuddlefish.packaging that verifies the resource URI hostnames being generated by cfx are valid:

  http://hg.mozilla.org/labs/jetpack-sdk/rev/9d51d77a9be5

For the time being, this actually makes it *impossible* to use cfx xpi, as it will always raise the following kind of error until this bug is fixed:

  ValueError: invalid resource hostname: jid0-ZrKOYC68LvzFxDYEDwbhyVAHx94@jetpack-

Not a terribly helpful error as far as cfx is concerned, but hopefully cfx users will never see it once we've fixed this bug. :)

Comment 16

[Brian Warner [:warner :bwarner]]

ok, so it sounds like the resource URL should be a derivative of the JID rather than being the JID itself, and that we're converging on three kinds of values:

 JID:  jid0-ZrKO..Hx94
 add-on ID: jid0-ZrKO..Hx94@jetpack
 resource prefix: jid0-zrko..hx94

The full JID will be used to confirm that the add-on was signed by the right key (specifically: the add-on declare a JID and will be bundled with a pubkey and a signature, we check that the XPI is correctly signed by that pubkey, then we hash the pubkey to derive the JID, and reject the XPI if the JID does not match the declared value; the verified JID is then made available as packaging.jetpackID and require("self").id ).

The add-on ID is used in the RDF file, and the app uses it to name the subdirectory of extensions/ that will contain the unpacked XPI.

The resource prefix is used in resource: URLs (and thus visible to require("self").url). It must be unique (but downcasing the JID still gets us 100 bits of entropy, so collisions are not a serious concern).

We can continue to use the resource prefix as a subdirectory name inside the "resources" directory of the unpacked XPI, or we could switch to something shorter. I don't believe we require this piece to be uniquely mapped to a specific add-on, since we're already inside an extensions/ADDONID/ subdir. Removing the prefix from these names (causing them to be just PACKAGE-TYPE, like "jetpack-core-lib") would also help our pathname-length budget a lot.
OTOH, we don't have to make this change right now: these prefix strings are an implementation detail of the SDK and can be changed later without being visible to add-on code (the return value of require("self").url("foo") would change, but nothing should be depending upon that).

Cool, I think this gives me enough information to finish that patch in the morning. Thanks!

Comment 18

[Brian Warner [:warner :bwarner]]

Attachment: set contract/classID to a GUID, not the JID

this one seems to work, but doctests are still broken (xpi.md, something about the JSON formatting)

Comment 19

[Brian Warner [:warner :bwarner]]

Attachment: set contract/classID to a GUID, not the JID

ok, this patch fixes the doctest failure

Comment 20

[Brian Warner [:warner :bwarner]]

ok, the patch is ready for review. I've successfully installed an XPI built with this one on both mindfield and ff3.6.3 .

Comment 21

[Atul Varma [:atul]]

Comment on attachment 446548 [details] [diff] [review]
set contract/classID to a GUID, not the JID

Awesome!

Comment 22

[Brian Warner [:warner :bwarner]]

Ok, landed, in 20cce16d1041. Thanks everybody!

Comment 23

[Myk Melez [:myk] [@mykmelez]]

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".