We embed library zlib. At the moment, it cannot be used directly from JavaScript, and in particular from workers. The objective of this bug is to add js-ctypes bindings for zlib. This can be a mentored bug, but it will be long and difficult and it requires quite a good knowledge of C and JavaScript. You have been warned.

Hi David, I have a couple of questions: In order to open a library with js-ctypes, one needs to provide a path to it. I am trying to load a zlibc library from within mozilla-central's code base (modules/zlib), but I'm having difficulty finding examples of how to locate its path. Would you know if there's a use case like that in the code base somewhere already? Thanks

^

Here's an IRC log that might be relevant : http://krijnhoetmer.nl/irc-logs/developers/20130502#l-1690

Latest update: bug 846410 might make replace this bug. I suggest you wait until some consensus has been reached on bug 846410.

Consensus was reached and the blocker is lifted.

I have started working on this, so I guess I can as well take that bug.

I have a first version that is able to perform compression and decompression. For the moment, decompression only works with an ArrayBuffer. I should make it work also with a pointer.

This patch exposes utility normalizeToPointer() in a better place of OS.File.

Finally, this patch exposes lz4 to OS.File.{read, writeAtomic}.

Added more fine-grained error handling, some documentation, more tests.

Ye forgotten tests for part 3.

Propagating refined error-handling. I took the opportunity to deprecate |bytes| in |OS.File.read(path, bytes, options)|, as it is rather counter-intuitive now that we have an options object.

Comment on attachment 822990 [details] [diff] [review] 4. lz4 tests Review of attachment 822990 [details] [diff] [review]: ----------------------------------------------------------------- This looks reasonable. I think we should include a test in which we try to decompress a pre-existing buffer (i.e. not one generated by compressFileContent) and compare it to its known decompressed contents. Similarly, we should also compress a known buffer and compare it to a pre-existing buffer. The buffers should be stored as files or somesuch. That way, the testsuite ensures that we don't change things without thinking about backwards compatibility. r=me with that change.

Comment on attachment 822989 [details] [diff] [review] 3. Bindings to lz4, v2 Review of attachment 822989 [details] [diff] [review]: ----------------------------------------------------------------- I'm a little surprised there's no "raw" LZ4 interfaces here, but I guess someone will add them if they seem useful. ::: toolkit/components/workerlz4/lz4.cpp @@ +1,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#define EXPORT_CTYPES What is this for? I don't see any bits in Compression.h that cater to this. ::: toolkit/components/workerlz4/lz4.js @@ +13,5 @@ > + > +let SharedAll = require("resource://gre/modules/osfile/osfile_shared_allthreads.jsm"); > +let Internals = require("resource://gre/modules/workers/lz4_internal.js"); > + > +let MAGIC_NUMBER = (new TextEncoder()).encode("mozLz4a"); Can you define this (raw bytes, etc.) within resorting to dragging in TextEncoder? Also, can you make this 8 bytes, probably by appending \0, so that content-length and the contents start on 32-bit aligned boundaries? @@ +17,5 @@ > +let MAGIC_NUMBER = (new TextEncoder()).encode("mozLz4a"); > + > +let BYTES_IN_SIZE_HEADER = ctypes.uint32_t.size; > + > +let HEADER_SIZE = MAGIC_NUMBER.byteLength + BYTES_IN_SIZE_HEADER; These constants should be, well, |const|. @@ +112,5 @@ > + EXPECTED_SIZE_BUFFER_TYPE.ptr).contents; > + let expectDecompressedSize = expectDecompressedSizeBuffer[0] > + + ( 1 << 8 ) * expectDecompressedSizeBuffer[1] > + + ( 1 << 16) * expectDecompressedSizeBuffer[2] > + + ( 1 << 24) * expectDecompressedSizeBuffer[3]; How about (edsb = expectDecompressedSizeBuffer): (edsb[0] + (edsb[1] << 8) + (edsb[2] << 16) + (edsb[3] << 24); I think this is slightly clearer. @@ +129,5 @@ > + // Decompress > + let success = Internals.decompress(inputPtr, bytes - HEADER_SIZE, > + outputBuffer, outputBuffer.byteLength, > + decompressedBytes.address()); > + if (!success) { Is it true that !success iff *decompressedBytes != outputBuffer.byteLength?

Comment on attachment 823050 [details] [diff] [review] 5. OS.File.{read, writeAtomic} support for lz4, v2 Review of attachment 823050 [details] [diff] [review]: ----------------------------------------------------------------- r=me once I can convince you that |mustBeCompressed| is superfluous. ;) ::: toolkit/components/osfile/modules/osfile_async_worker.js @@ +89,5 @@ > // built-in mechanism for uncaught errors, although this mechanism > // may lose interesting information. > + > + // However, rewrite interesting information to take into account > + // modules. Did you mean to include this? If so, can you split it out to a separate bug or patch? ::: toolkit/components/osfile/modules/osfile_shared_front.jsm @@ +312,5 @@ > + * fields: > + * - {number} bytes An upper bound to the number of bytes to read. > + * - {string} compression If "lz4" and if the file is compressed using the lz4 > + * compression algorithm, decompress the file contents on the fly. > + * - {bool} mustBeCompressed If |true| and if |compression: "lz4"| Why would we ever want |compression: "lz4"| and |mustBeCompressed: false|?

(In reply to Nathan Froyd (:froydnj) from comment #17) > ::: toolkit/components/osfile/modules/osfile_shared_front.jsm > @@ +312,5 @@ > > + * fields: > > + * - {number} bytes An upper bound to the number of bytes to read. > > + * - {string} compression If "lz4" and if the file is compressed using the lz4 > > + * compression algorithm, decompress the file contents on the fly. > > + * - {bool} mustBeCompressed If |true| and if |compression: "lz4"| > > Why would we ever want |compression: "lz4"| and |mustBeCompressed: false|? I am thinking of a transition period during which some files may be either uncompressed (if they have been produced by previous versions of Firefox) or compressed (if they have been produced by the latest version). But yes, the more I think about it, the more I realize that we should change the file name when we upgrade to a lzived version. Consider me convinced :)

Comment on attachment 822815 [details] [diff] [review] 2. Build system stuff Review of attachment 822815 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/workerlz4/moz.build @@ +13,5 @@ > + 'lz4.js', > + 'lz4_internal.js', > +] > + > +CPP_SOURCES += [ You've been bitrotted. It's SOURCES, now.

Applied feedback. In particular, I strengthened lz4_decompress for the case in which the number of bytes was lower than the expected number of bytes. I don't think that this can happen, but one never knows.

Same thing, minus typo.

Added reference test.

Added missing data file.

Removed mustBeCompressed, exception-handling-code-that-is-not-specific-to-this-bug.

Comment on attachment 824674 [details] [diff] [review] 5. OS.File.{read, writeAtomic} support for lz4, v3 There are still a few traces of mustBeCompressed here.

Removed references to now disappeared option mustBeCompressed.

Try: https://tbpl.mozilla.org/?tree=Try&rev=c914d5f85d06

Adding forgotten commit message.

Unbitrotten

Try: https://tbpl.mozilla.org/?tree=Try&rev=0f9fcbb1d96e

Removing reference to a non-existing module. Try: https://tbpl.mozilla.org/?tree=Try&rev=27c6f1ddc894

https://hg.mozilla.org/integration/fx-team/rev/ccae8b3f0a7d https://hg.mozilla.org/integration/fx-team/rev/4ede802f8dbb https://hg.mozilla.org/integration/fx-team/rev/844860e32631 https://hg.mozilla.org/integration/fx-team/rev/12cebd8b5669 https://hg.mozilla.org/integration/fx-team/rev/8898b6c66941

Android did *not* care for this. Backed out. https://hg.mozilla.org/integration/fx-team/rev/a8fe75687113 https://tbpl.mozilla.org/php/getParsedLog.php?id=30330422&tree=Fx-Team

Patch 2 doesn't apply due to glandium's recent SHARED_LIBRARY_LIBS removal. Please post an updated patch (probably with him taking another look at it).

Comment on attachment 828616 [details] [diff] [review] 2. Build system stuff, v3 Review of attachment 828616 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/workerlz4/tests/moz.build @@ +5,5 @@ > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +MODULE = 'test_workerlz4' > + > +XPCSHELL_TESTS_MANIFESTS += ['xpcshell/xpcshell.ini'] Does this need its own moz.build?

Adapted to the latest changes in our build system.

https://hg.mozilla.org/integration/fx-team/rev/1b339f171b0a https://hg.mozilla.org/integration/fx-team/rev/224d0d4df434 https://hg.mozilla.org/integration/fx-team/rev/c0f9bfe37d74 https://hg.mozilla.org/integration/fx-team/rev/cbf7e38a49aa https://hg.mozilla.org/integration/fx-team/rev/bd7733e5d5cf

And as predicted by Ms2ger, a bustage follow-up. https://hg.mozilla.org/integration/fx-team/rev/236a0eba3a8a

Weird. For some reason, it worked for me, so I didn't think of re-running this on Try.

Backed out for various failures. Please give this a full Try run before requesting checkin again. Also please refresh the build system patch while you're at it. https://hg.mozilla.org/integration/fx-team/rev/2d4fd5a493b1 OSX Marionette bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=31324613&tree=Fx-Team OSX Jetpack timeouts: https://tbpl.mozilla.org/php/getParsedLog.php?id=31332695&tree=Fx-Team Android reftest-2 unexpected passes: https://tbpl.mozilla.org/php/getParsedLog.php?id=31327267&tree=Fx-Team

(In reply to Ryan VanderMeulen [:RyanVM UTC-5][Back 2-Dec] from comment #38) > And as predicted by Ms2ger, a bustage follow-up. > https://hg.mozilla.org/integration/fx-team/rev/236a0eba3a8a Investigating. I have difficulties believing that any of these issues is related to my patches, though.

VYV03354, do you have any idea why this could break the Android reftest-2?

Maybe WebFont loading are broken, then both files are rendered with the same system default font? (Variation selectors will be ignored if the font doesn't support.)

So, the Jetpack problem is actually due to a problem in the Jetpack test runner (bug 952335). Until someone in the Jetpack team takes time to merge the Jetpack test runner and the test runner for the rest of mozilla-central, here is a workaround that simply delays linking to libxul until this is actually needed.

Comment on attachment 8356548 [details] [diff] [review] 6. Lazy loading Review of attachment 8356548 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/osfile/modules/osfile_shared_allthreads.jsm @@ +884,5 @@ > /** > + * Representation of a native library. > + * > + * The library is opened lazily, during the first call to |open| or > + * whenever accessing one of its methods for the first time. Which |open| is this? You mean the first reference to |.library|? I don't know that the bit about its methods makes sense with the current code...maybe "when calling one of its methods declared through declareLazyFFI"? @@ +943,5 @@ > + * @param {ctypes.abi} abi The abi to use, or |null| for default. > + * @param {Type} returnType The type of values returned by the function. > + * @param {...Type} argTypes The type of arguments to the function. > + */ > + declareLazyFFI: function(object, field, ...args) { It would be nice to unify this with the existing |declareLazyFFI| function supported in this module and/or transition everything over to using |Library|. Followup bug?

Applied feedback, filed followup as bug 952335.

Try: https://tbpl.mozilla.org/?tree=Try&rev=935332f100ab

Same try after pull: https://tbpl.mozilla.org/?tree=Try&rev=1c8c54dee20f Looks to me like it's working. Let's try landing this once again.

Latest build system stuff.

https://hg.mozilla.org/integration/fx-team/rev/d043b187d96e https://hg.mozilla.org/integration/fx-team/rev/1129e0e63c81 https://hg.mozilla.org/integration/fx-team/rev/d0ab9e475844 https://hg.mozilla.org/integration/fx-team/rev/5c4f21043ce1 https://hg.mozilla.org/integration/fx-team/rev/52f2b7583b97 https://hg.mozilla.org/integration/fx-team/rev/a270eb448899

https://hg.mozilla.org/mozilla-central/rev/d043b187d96e https://hg.mozilla.org/mozilla-central/rev/1129e0e63c81 https://hg.mozilla.org/mozilla-central/rev/d0ab9e475844 https://hg.mozilla.org/mozilla-central/rev/5c4f21043ce1 https://hg.mozilla.org/mozilla-central/rev/52f2b7583b97 https://hg.mozilla.org/mozilla-central/rev/a270eb448899

Please document this awesome new feature!