Dave Garrett Reporter
	Description • 9 years ago

https://addons.mozilla.org/addon/flagfox

Flagfox 5.0.15 XPI size prior to signing is 724 KiB. After AMO signs it, the new XPI is now a 897 KiB! 173 KiB is a lot of added bloat to add. Breaking this down, there are two sources:

1) You re-compressed the XPIs badly. Every single file in the new archive has a worse compression ratio then when I uploaded it. This adds about 20% to the XPI size, which in my case is 150 KB. :(

2) The signature is quite large because it provides multiple separate digests for every single individual file in the archive. For me, that's 440 files. (250 flag icons and 36 locales means lots of files) The compressed size of my signature is 23 KB. (not horrible, but it can be improved)

First and foremost, you need to fix your compression, preferably by not redoing it. I've fairly optimized my build process to reduce installer size, and this kinda throws a wrench in that. If you must re-compress the whole thing, please use a better zipper. I use 7-zip (Linux version via my package manager), as it consistently produces ZIPs with a far better compression ratio than others. Here's an excerpt from my build script:

7z a -tzip -mx=9 -r $buildLocation $tmpDir/* -x!*.png
7z a -tzip -mx=0 -r $buildLocation -i!$tmpDir/*.png
7z t $buildLocation

First I compress everything other than PNGs at max level. Next I add the PNGs without compression (store instead of deflate). I do this because I can far better optimize their file sizes with pngcrush or other image optimization, so double compressing can't help here. (PNGs use deflate, too) Lastly I test, just to be safe.

If you're going to take responsibility for optimizing everyone's XPIs, do the above. If not, please just append the signature at max compression level. ('7z a' adds to an existing ZIP or creates a new one if it does not exist) Other zippers can append without re-compressing, as well.

If I re-compress my signature files, I can shave off about half a kilobyte from the compressed size. Not a lot, but there's no reason to have this waste, either.

Next up, is there really any value to having both MD5 & SHA1 digests? MD5 is garbage at this point. Frankly, you shouldn't be using SHA1 either, and should be using just SHA256. (assuming there isn't a bad compatibility issue here) Whilst SHA256 is bigger, just doing it would be notably more compact than MD5+SHA1, more secure, and faster to verify.

Lastly, it appears to be using RSA. If it was using some form of ECC it would have a much smaller key size. This would only save a few kilobytes, though it might improve verification performance as well. Not sure if this is something you'd actually be able to change easily, though.

Footnote:
You probably need to add an addon XPI signing component to BMO, as there's not really a good place to file this under the AMO product.

	Dave Garrett Reporter
	Comment 1 • 9 years ago

Oh, and to state an important point: This isn't just about download size. XPI files have to be loaded by Firefox so this affects every addon load, and thus every Firefox startup. More efficient compression means smaller files and less to load into memory.

	Dave Garrett Reporter
	Comment 2 • 9 years ago

Lord... you signed every single version of Flagfox back to Feb 2011?! I thought you were just going to be auto-signing the latest version? That is a lot of wasted effort there. This is a separate issue, but you really should revert that. I know I am an odd case, due to my monthly updates, but signing 52 versions for one addon seems pointlessly messy.

	Kris Maglione [:kmag]
	Comment 3 • 9 years ago

You have two separate bugs here. I'm no interested in optimizing the signature or manifest files for size or compressibility. But the security of the checksums is a different question, for which you might consider filing a bug.

As for the compression levels, there are a few points:

1) For the moment, we're still storing the mozilla.rsa file as the first entry in the archive, for the sake of legacy tools, so we can't just append files to the archive. We also need to remove old signatures, in any case. It might be possible to modify a copy of the original archive without recompressing everything, but I'm not sure it's worth the effort.

2) It would probably be a good idea to blacklist extensions of compressed binary formats, including most image formats, and store them without compression.

	Dave Garrett Reporter
	Comment 4 • 9 years ago

What legacy tools? The only thing it needs to work with is Firefox and other supported Gecko-based applications. 20% filesize bloat is not justified for backwards compatibility with some unspecified and admittedly "legacy" (aka obsolete) software.

There's also no reason you can't compress the signature, then append the original files. You'd just need to do so with a compressor that isn't bad (most are). Again, 7z is by far the best at this.

	Kris Maglione [:kmag]
	Comment 5 • 9 years ago

The point is that it's the decision we've made, and changing it would require non-trivial research and discussion.

(In reply to Dave Garrett from comment #4) 
> There's also no reason you can't compress the signature, then append the
> original files.

There are reasons we can't just compress the signature and append them to the original file. As I've already said, regardless of the position of manifest.mf in the archive, we also need to remove any old signature. The python zipfile module, with the signing client currently uses, is not capable of this. We could rewrite it to use something else, but there aren't many viable options aside from calling out to an external binary and, as I said above, I'm not convinced it's worth the effort at this point.

	Dave Garrett Reporter
	Comment 6 • 9 years ago

I just decompressed a recent Firefox profile backup and compared some file sizes between signed and unsigned XPIs for my installed addons. All are of course larger now. My other addon, Config Descriptions, actually got its filesize doubled, but that's just because it was tiny. You'd have to use ECC to get smaller keys to deal with that. Flagfox is the worse off after this process, though.

Here's where the bad compression really hits Flagfox: the IPDB files.

file     uncompressed   old     new     bloat
ip4.db   637KB          233KB   330KB   97KB  (+42%)
ip6.db   359KB          75KB    125KB   50KB  (+67%)

ouch

Your compressor is borderline broken there. Do you actually have it set to max level, or is it just old? (most ZIP compressors are just bad; it's an old format, so hardly anyone bothers to optimize it)

	Dave Garrett Reporter
	Comment 7 • 9 years ago

(In reply to Kris Maglione [:kmag] from comment #5)
> The point is that it's the decision we've made, and changing it would
> require non-trivial research and discussion.
[...]
> I'm not convinced it's worth the effort at this point.

Well, it's broken and causes a rather large bloat for Flagfox. I have about 740000 users (weekly peak). Add new user downloads to monthly updates, and that's in the order of 800k downloads a month. That's around 115GB/month of bandwidth wasted. That's a waste of your bandwidth as well as of my userbase. That's a waste of RAM to load the thing for all of them as well. Please don't forsake my addon just because most came out of this process with less waste. This signing process was sold to the community on the grounds that it would not break our addons, and whilst it still works, it's clearly broken.

	Dave Garrett Reporter
	Comment 8 • 9 years ago

Based on some research, the answer to my question in comment 6 is: no. It looks like the python zipfile module is missing the ability to even configure its compression level. (wow) It appears it uses the default zlib level, which is 6, instead of the maximum level, which is 9. So, even not counting the bad compressor, python is mucking it up. :(
https://stackoverflow.com/questions/27526155/python-zipfile-how-to-set-the-compression-level
https://stackoverflow.com/questions/16485573/what-exactly-is-default-compression

XPI signing is a rare action; speed on the server shouldn't be a (primary) concern. It should at least be fixed to run at proper full compression, if not using a high-efficiency compressor.

	Dave Garrett Reporter
	Comment 9 • 9 years ago

Ok, I decided to follow the code:

https://github.com/mozilla/olympia/blob/master/lib/crypto/tasks.py#L140
https://github.com/mozilla/olympia/blob/master/lib/crypto/packaged.py#L131
https://github.com/mozilla/olympia/blob/master/lib/crypto/packaged.py#L84
https://github.com/mozilla/signing-clients/blob/master/signing_clients/apps.py#L340
https://hg.python.org/cpython/file/3.4/Lib/zipfile.py#l1443
https://hg.python.org/cpython/file/3.4/Lib/zipfile.py#l598
uses zlib.Z_DEFAULT_COMPRESSION
https://docs.python.org/3.4/library/zlib.html#zlib.compressobj
default compression level is 6 (min 0; max 9)

It looks like fixing this requires either changing zlib.Z_DEFAULT_COMPRESSION to 9, or modifying zipfile.py to use an integer literal 9 when calling zlib.compressobj(,,). Not sure where the best way to patch this is.

Bottom line is that if you're going to continue to require usage of a compression algorithm from 1993, please turn it on fully.

	Kris Maglione [:kmag]
	Comment 10 • 9 years ago

I've talked about this with a few people. We came to the conclusion that there's not much point in trying to change the zlib compression level, since it makes very little difference in the vast majority of cases.

We're considering switching to 7-zip, since we already use it for Firefox releases, but it's not an immediate priority.

	Dave Garrett Reporter
	Comment 11 • 9 years ago

Note that for my specific case with Flagfox, I've taken measures to reduce the hit from this issue. The IPDB files were getting steadily larger anyway, due to that whole "we're out of IPv4 addresses" thing making the data increasingly complex, so shrinking them sounded like a good idea regardless. I came up with a context-specific compression scheme to shrink those 2 files by more than half before ZIPing and they also ZIP much better without needing an efficient compressor to get big compression. (optimizing the decompressor in JS was a PITA, but I got it down to a few milliseconds, lazy loaded, even on older hardware; 3 cheers for the latest JS JIT improvements)

There's still notable waste from this bug, but it's far less drastic for Flagfox 5.1+ now. (just uploaded today and passed editor review a couple hours ago)

All addons do have waste from this, and there's undoubtedly others that got hit worse than average out there too, so fixing your signer to use proper compression is still something I would encourage you to not put at the bottom of the todo list, even if it doesn't belong at the top. ;)