733051 - add testcases for ByteArray lzma compress/uncompress support

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Dan Schaffer]

Attachment: testcase for ByteArray lzma

add testcases into tamarin to test ByteArray lzma compress and uncompress..

Comment 1

[Dan Schaffer]

testing in tamarin sandbox.  I reviewed the tests they passed locally.

Comment 2

[Tamarin Bot]

changeset: 7252:d98bf90b2f73
user:      Dan Schaffer <[e-mail]>
summary:   bug 733051: add testcase for ByteArray lzma compress/uncompress  on behalf of Liping Zhao (r=dschaffe)

http://hg.mozilla.org/tamarin-redux/rev/d98bf90b2f73

Comment 3

[Dan Schaffer]

all tests passed after submitting bug.  marking as fixed.

Comment 4

[Dan Schaffer]

reopening tests.  I did some cleanup of the LzmaCompress test.  I added support for running zlib and/or lzma on the acceptance test.  The update should give us better coverage and improved output from the test.

Comment 5

[Dan Schaffer]

Attachment: patch to add zlib support and add better test messages

Comment 6

[Felix S. Klock II [:pnkfelix, :fklock]]

To check for compatibility with third-party utilities, we should add some pre-compressed data to the test suite and make sure we can uncompress it.  (AFAICT right now we are solely checking that we can round-trip, which is necessary, but not sufficient.)

Comment 7

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch I v1: add some tests constructing ill-formed inputs

Comment 8

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch J v1: test that third-party lzma compression is also handled

Dan: I need help on this one: What is the appropriate way to suck in input from external files into a test?  Or is one just supposed to transcribe the bytes into the .as code itself?

Comment 9

[Dan Schaffer]

Comment on attachment 627266 [details] [diff] [review]
patch J v1: test that third-party lzma compression is also handled

using File.read or File.readByteArray is ok.  Make sure you add a dir.asc_args file containing: merge| -nooptimize -import $SHELLABC

Comment 10

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Dan Schaffer from comment #9)
> using File.read or File.readByteArray is ok.  Make sure you add a
> dir.asc_args file containing: merge| -nooptimize -import $SHELLABC

Hmm.  Is it "-nooptimize", or is it "-no-optimize" ?

% tail -1 as3/sampling/dir.asc_args
merge| -no-optimize -import $SHELLABC

% tail -1 as3/ShellClasses/dir.asc_args
merge| -nooptimize -import $SHELLABC


(Or are the two synonymous?)

Comment 11

[Felix S. Klock II [:pnkfelix, :fklock]]

Based on my reading of parseRootConfigFiles (in runtestBase.py), I'm thinking that it was supposed to be "-no-optimize", and all the existing uses of "-nooptimize" are errors.

There are not many uses of it either way, as you can see from this shell command sequence:

% find . -name '*.asc_args' -exec grep -nH -- '^[^#]*-no' {} \;
./as3/sampling/dir.asc_args:6:merge| -no-optimize -import $SHELLABC
./as3/ShellClasses/dir.asc_args:6:merge| -nooptimize -import $SHELLABC
./as3/Types/Float/nonstrict/dir.asc_args:6:merge| -no-strict
./as3/Types/Float4/nonstrict/backwards_compat_4616.as.asc_args:6:merge| -no-abcfuture
./as3/Types/Float4/nonstrict/dir.asc_args:6:merge| -no-strict
./as3/Workers/dir.asc_args:6:merge| -nooptimize -import $SHELLABC
./regress/vector_domain_bug.as.asc_args:6:merge| -nooptimize -import $SHELLABC
./spidermonkey/dir.asc_args:6:merge| -no-AS3

Comment 12

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Dan Schaffer from comment #9)
> using File.read or File.readByteArray is ok.  Make sure you add a
> dir.asc_args file containing: merge| -nooptimize -import $SHELLABC

Dan and Felix had a follow-up chat about this.  Felix was not clear on what these options were for.

Dan does not think the "-nooptimize" (sic; see comment 11) is necessary.

The "-import $SHELLABC" option *is* necessary, but only to accommodate builds that run asc.jar in strict-mode (i.e. via the "-strict" option).  The problem here is that running the acceptance tests in strict-mode does not necessarily work; for me putting "-strict" into the asc_args yields:

[Compiler] Error #1120: Access of undefined property float.
   ./shell.as, Ln 245, Col 18:
               if ( float.abs(actual-expect) < float(0.000001) ) {
   .................^

[Compiler] Error #1180: Call to a possibly undefined method float.
   ./shell.as, Ln 245, Col 45:
               if ( float.abs(actual-expect) < float(0.000001) ) {
   ............................................^

This is probably a bug in shell.as. but then again, brbaker specifically put in this changeset to support '-strict' mode; see Bug 706148, comment 4.  So I don't know what to do at this point; can we remove the two "float."'s all together, and make the computation match the one from a few lines above?  Or does our type system not allow that?

Comment 13

[Felix S. Klock II [:pnkfelix, :fklock]]

(in response to comment 12: maybe I should re-open Bug 706148?)

In any case, further discussion of that issue really does not belong on this ticket; I just wanted to explain the purpose of the "-import $SHELLABC"

Comment 14

[Felix S. Klock II [:pnkfelix, :fklock]]

Ingo writes:

"""
After reviewing the ATS test cases, I added two additional tests to acceptance/as3/ByteArray/ByteArrayLzma.as:
1) compressWithZlibUncompressLzma - compress with ZLIB try to decompress with LZMA - ATTENTION: this lead to an (reproducible) avmshell crash on my machine 
2) uncompressWithoutCompressionLzma- try to uncompress an ByteArray with LZMA which only contains some text

[...]

This crashes the avmshell: It seems that compressWithZlibUncompressLzma is really useless in what it does (uncompress an uncompressed ByteArray with some bytes content), but it seems that this case is not properly handled in the avm. I'm expecting an IOError (that's at least my understanding of what should be returned in this case (http://help.adobe.com/en_US/FlashPlatform/reference/actionscript/3/flash/utils/ByteArray.html#uncompress())), but instead the avmshell crashes.
"""

Comment 15

[Felix S. Klock II [:pnkfelix, :fklock]]

I'm working on replicating Ingo's crash.  I am not sure he correctly identified the root of the problem; in particular, Ingo's shelved test has a typo that causes a TypeError to be signalled from uncompressWithoutCompressionLzma(), which might be the actual problem here.

Ingo: You need to change your code in this manner (pidgen unified diff format):

 function uncompressWithoutCompressionLzma()
 {
     var byteArray:ByteArray = new ByteArray();
     byteArray.writeUTF("UNCOMPRESS TEST");

-    var uncompressedLengthBefore : uint = byteArray.length();
+    var uncompressedLengthBefore : uint = byteArray.length;

Comment 16

[Felix S. Klock II [:pnkfelix, :fklock]]

Submitted, to Tamarin Depot in CL 1069582, the following tests:

* My proposed tests for extreme-size cases and for fuzzing

* Ingo's additional test of "round trip" from ZLIB through LZMA

* Ingo's additional test of LZMA uncompress of already uncompressed input (which in that particular case throws a MemoryError, I believe because the input he happened to chose indicates a large size when uncompressed).

* My proposed tests for sanity-checking against third-party LZMA.jar compressed input


(I usually prefer to tease these things out into distinct changesets, but these are all semi-related tests, and I really want to put this thing to bed, so I'm short-cutting my usual process.)

Comment 17

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch S: skip ByteArrayLzmaThirdParty on android

Brent (or any other QE): please sanity check my understanding of why the test fails on android (see http://tamarin-jenkins.corp.adobe.com/view/Sandbox/job/sandbox-test/lastFailedBuild/platform=android-test/ for reference) and whether we must skip it, (or if I'm totally wrong and there is some other reason that this test is failing, and that I probably need to fix it).

(I'm going to push this patch with your review pending, which adds the skip entry for this isolated test, so that the builds get clean again.)

Comment 18

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Felix S Klock II from comment #16)
> Submitted, to Tamarin Depot in CL 1069582, the following tests: [...]

And integrated to FlashRuntime Main in CL 1069594

Comment 19

[Dan Schaffer]

Attachment: minor cleanup to lzma testcase

Comment 20

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: FB project E: Flash Builder project illustrating Embed[..] and lzma compress/decompress (it seems to work)

Comment 21

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: swf F: swf generated from FB project E

Comment 22

[Felix S. Klock II [:pnkfelix, :fklock]]

Adobe employees may also want to check out a related blog post from me here:

  https://zerowing.corp.adobe.com/x/bo9zKQ

Comment 23

[Tamarin Bot]

changeset: 7416:e2d1f8e9f2d6
user:      Felix Klock II <[e-mail]>
summary:   Bug 733051: various lzma acceptance tests (p=fklockii, irichter; r=dschaffe).

http://hg.mozilla.org/tamarin-redux/rev/e2d1f8e9f2d6

Comment 24

[Tamarin Bot]

changeset: 7417:4de002cd5b14
user:      Felix Klock II <[e-mail]>
summary:   Bug 733051: skip ByteArrayLzmaThirdParty as workaround for android (r pending=brbaker).

http://hg.mozilla.org/tamarin-redux/rev/4de002cd5b14

Comment 25

[Tamarin Bot]

changeset: 7423:24ee01125d63
user:      Dan Schaffer <[e-mail]>
summary:   bug 733051: minor cleanup to ByteArray lzma testcases (r=brbaker)

http://hg.mozilla.org/tamarin-redux/rev/24ee01125d63