564248 - Unify ByteArray between Tamarin and Flash Player

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P3)

Description

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

Some fixes for OOM conditions have been put into the Flash player's avmglue/ByteArrayGlue.cpp (see at least changes put in on 4/19/10); these need to be propagated over to the tamarin-redux shell/ByteArrayGlue.cpp

Comment 1

[Gavin]

If we fix the cases were mmgc is triggering OOM on canfail allocates, then there is no need to port the change to Grow that tests for the size before doing an allocate. In fact, that code should be removed from the player code once this the mmgc fix is in place. There may be other changes that do need syncing.

Comment 2

[Steven Johnson]

We should upgrade this bug to be "unify ByteArray between VM and Flash" -- having two similar-but-not-identical classes is silly, and moving it into the VM could open some possibilities for smarter JIT code.

Comment 3

[Steven Johnson]

Taking a look at this... one

Comment 4

[Steven Johnson]

Taking a look at this... one interesting wrinkle is that the Flash/AIR ByteArray.as uses ASC-compile-time configuration to make things conditional to Flash or AIR, ie, instead of

[API(CONFIG::AIR)]

it uses


CONFIG::AIR
{
...
}

(where CONFIG::AIR and CONFIG::FLASH are true or false for each build)

Obviously these values aren't defined for us; we could solve that easily enough, but the fact of this conditional compilation gives me pause...

Comment 5

[Jeff Dyer]

you should be able to replace the conditional blocks with api versioning meta data on the definitions they are hiding, with one complication. where there are two different definitions (e.g. function compress) with the same name (one for air and one for fp) you'll need to make one definition work. in the case of compress and uncompress, it seems like this is doable since the defaults for air are compatible with the behavior for fp. just delete the FP version and don't worry about versioning at all.

Comment 6

[Steven Johnson]

Attachment: Step 1: remove readFile/writeFile

Baby Step #1 towards unification: AIR/Flash don't have the readFile/writeFile methods, so neither should ours. Use the File methods instead.

Not ready for review because apparently a handful of fuzzer bugs are based on abcdump, which used the method, and since it's missing, the fuzzer files don't resolve properly anymore... not sure of the best fix there, but the rest of the changes are mechanical.

(Query: should we nuke the never-used readOnly flag? Change names while we're at it? etc)

Comment 7

[Edwin Smith]

Comment on attachment 479261 [details] [diff] [review]
Step 1: remove readFile/writeFile

re: readFile -- cool, these should be File api's anyway.

re: readOnly flag -- nuke if dead.

Comment 8

[Steven Johnson]

Attachment: readFile/writeFile v2

renamed to File.readByteArray and removed useless arg.

Still can't land due to the handful of fuzzed files that rely on "ByteArray.readFile" existing -- not sure of the best way to deal with these, as re-fuzzing them might not tickle the same bug in the same way, and I'm not eager to spend the time trying to re-create the bugfiles by hand -- can anyone think of a sneaky way to keep them working? (disassemble, hack the names, reassemble... or provide some stub version of ByteArray for these that provides "readFile"... or... whatever)

Comment 9

[Edwin Smith]

Could we leave the readfile api in ByteArray in avmshell?  conditionally compile it so its not present in flash player builds.

Comment 10

[Steven Johnson]

(In reply to comment #9)
> Could we leave the readfile api in ByteArray in avmshell?  conditionally
> compile it so its not present in flash player builds.

Ewww. Yeah, I guess we could.

Comment 11

[Edwin Smith]

the other option is to go back to the shell that had fuzz bugs, and find a different test case to expose the same bug as the fuzzed abcdump.  just doesn't seem worth the trouble if conditional compilation only has to be worried about in avmshell.

Comment 12

[Steven Johnson]

I could special-case these tests to preload an abc that hacks the Class prototype, eg

package {
    import avmplus.File
    Class.prototype.readFile = function(f) { return File.readByteArray(f); }
}

Comment 13

[Steven Johnson]

Attachment: readFile/writeFile v3

Now with fixed acceptance tests: for a few tests, we prepend the abc execution with another ABC that hacks the class prototype:


package {
    import avmplus.File
    import flash.utils.ByteArray
    ByteArray.readFile = function(f) { return File.readByteArray(f); }
}

Comment 14

[Steven Johnson]

Attachment: Adding the one binary file

hg diff doesn't include binary files; adding it here

Comment 15

[Tommy Reilly]

Note that I think we want to move ByteArray.cpp into extensions to live next to Dictionary/Sampler?   The way xcode looks up files though I think you have to remove ByteArray.cpp from p4 at the same time.

Comment 16

[Steven Johnson]

(In reply to comment #15)
> Note that I think we want to move ByteArray.cpp into extensions 

I think we want to move it (and Dictionary, etc) into core... they are all already tight enough that they can't be removed.

But right now, project file hell means that moving files around is the last thing I'm going to do -- get everything else right first, then deal with that mess.

Comment 17

[Steven Johnson]

Attachment: Patch #2, v1: move ByteArray and DataIO into core

I really wanted to avoid the pain of moving these from their present location, but subsequent patches will be even more painful if we don't bite the bullet now. This is a messy patch because things are getting moved around, but the actual change is pretty minimal.

Comment 18

[Steven Johnson]

Attachment: Patch #3, v1: hooks for multibyte and AMF

The Flash/AIR version of ByteArray allows you to read/write strings in non-unicode format, in a system-dependent (codepage way); it also allows you to read/write object graphs using AMF0 and AMF3 formats.

We don't want to pull all that code into the VM at this time (maybe never), so this adds some minimal hooks to AvmCore that can be overridden in Flash/AIR to maintain existing behavior. (We'll use these hooks in a subsequent patch)

Comment 19

[Steven Johnson]

Attachment: Patch #4, v1: add IDataInput and IDataOutput

Adds the IDataInput and IDataOutput interfaces (visible as AS3 level).

Comment 20

[Steven Johnson]

(Yes, I know tabs/spaces are botched -- assume those will all get fixed before landing)

Comment 21

[Lars T Hansen]

Comment on attachment 479644 [details] [diff] [review]
Patch #2, v1: move ByteArray and DataIO into core

I know this is unhelpful scope creep, and maybe it's a minor point, but:

(a) A directory named "core" is not a good idea in the first place and we
    should think about how to retire it, in the long term

(b) Our "core" directory already has a lot of stuff in it (too much)

(c) A layered design is better than a ball-of-mud, but our core directory
    is approximating the ball-of-mud

If we're moving files, could we consider creating a "library" or (less good) "builtin" or (awful, but justifiable) "glue" directory into which we could move our implementations of AS3-visible classes?

Comment 22

[Steven Johnson]

(In reply to comment #21)
> I know this is unhelpful scope creep, and maybe it's a minor point, but:

It's definitely unhelpful :-)

But seriously, adding any new source directories is a major major MAJOR integration pain at present with the Flash/AIR sourcebase; until that rathole is cleared up, we need a moratorium on new folders in avmplus. Seriously. (Unless you want to do the necessary integration work next time.) Yes, it sucks that external factors of that sort limit optimal code arrangement, but that's the current status quo.

Comment 23

[Edwin Smith]

given that situation, i'd nominate "extensions" as our library/builtin/glue directory.  It might be easier to move things in/out of that dir so it has the right scope, than to create a new dir.

Comment 24

[Steven Johnson]

This is turning into a nasty can of worms...

I'd still like feedback on patches #2 - 4, but I think I need to rethink the approach; the problem is that the "right" fix really needs (I think) to have ByteArray and DataIO to be brought into avmplus/core... but that means we can't land it safely until 100% of all changes are brought over (otherwise FRR would break due to confusion between two similar-but-different ByteArray/DataIO in the same namespaces)

I think I need to ponder a way to continue it in baby steps, and not have to get everything right from the start...

Comment 25

[Gavin]

I'm looking for a place to add Telemetry. Would extensions be the place? 
It will have an AS interface eventually, but so far it doesn't.

Comment 26

[Gavin]

Moving AMF into the VM might be useful for telemetry. It does a pretty good job of compressing objects and is already supported by many other languages. I was surprised to find that it wasn't a part of the VM, given that we have published the format.

Comment 27

[Steven Johnson]

Comment on attachment 479489 [details] [diff] [review]
readFile/writeFile v3

review ping...

Comment 28

[Steven Johnson]

Comment on attachment 479644 [details] [diff] [review]
Patch #2, v1: move ByteArray and DataIO into core

removing review requests for now and obsoleting; I'm going to take a different approach to unification instead: trying to get the Flash/AIR version into a shape that can simply replace this one.

Comment 29

[Steven Johnson]

Comment on attachment 479489 [details] [diff] [review]
readFile/writeFile v3

Load-balancing to Werner at Edwin's request

Comment 30

[Werner Sharp]

Comment on attachment 479489 [details] [diff] [review]
readFile/writeFile v3

Looks fine.  I don't think we need builtin.* files in patches...they are impossible to review and just clutter up the patch (1.3 MB+)

Comment 31

[Steven Johnson]

(In reply to comment #30)
> Looks fine.  I don't think we need builtin.* files in patches...they are
> impossible to review and just clutter up the patch (1.3 MB+)

Agreed, I normally try to exclude those.

Comment 32

[Steven Johnson]

Comment on attachment 479489 [details] [diff] [review]
readFile/writeFile v3

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

Comment 33

[Rick Reitmaier]

(In reply to comment #32)
> Comment on attachment 479489 [details] [diff] [review]
> readFile/writeFile v3
> 
> http://hg.mozilla.org/tamarin-redux/rev/d2f31f658c66

This is just code reorg, correct?

Comment 34

[Steven Johnson]

Yep.

Comment 35

[Steven Johnson]

Attachment: Chunk #1: Lots of changes from Flash/AIR

Lots more changes from the Flash/AIR version of ByteArray, to bring them even closer together... apologies in advance for the messiness of this; it arguably needs to be split into smaller chunks for reviewability, but doing so proved to be a big mess; I would suggest you simply review ByteArray as an all-new class (roughly conforming to the API of the old ByteArray/ByteArrayFile duo) and the rest of the changes as a set of patches.

The main points:

-- ByteArray and ByteArrayFile collapsed into one class

-- IDataInput and IDataOutput added at the AS3 level (ByteArray now implements these)

-- rewrote the ByteArray::SubscriberRoot linked list to use a plain List<>, as the linked list assumed we lived in GC memory (which isn't always true, it's legal to use a ByteArray on the stack)

-- added a Copy-On-Write facility to ByteArray, needed by the Player.

-- added code to tell GC about "external" memory being allocated or deleted (necessary for proper GC metrics and OOM handling)

-- ByteArray::Write didn't handle the possibility of writing to itself, with potentially crashy results.

-- unioning in various DataIO method cleaned up for better aliasing

-- General cleanup and streamlining of existing allocation code, pruning of unused or deprecated-wrapper methods, conversion to C99 types, inlining where appropriate, etc

-- doxygen comments in ByteArray.as updated

-- There are bottlenecks in Toplevel for dealing with multibyte string conversion and AMF serialization; the intent is that there will be overrides in PlayerToplevel to provide equivalent functionality to what the Player already has. I considered providing useful stubs for these, but decided it wasn't worth it at this point in time.

Known deficits & limitations that will be addressed in future patches:

-- DataInput, DataOutput, and ByteArray were all moved from namespace avmshell to avmplus, but the source files still live in "shell"; this is by design, as I need to keep the source code where it is for now to avoid integration nightmares. As a result, there are some FIXME's regarding include files and enum declarations; please bear with me for now on these, as the intent is that they will be short-lived (once unification is done, these files will be moved around and the FIXME's can be nuked).

Comment 36

[Werner Sharp]

Comment on attachment 481113 [details] [diff] [review]
Chunk #1: Lots of changes from Flash/AIR

Wow, a lot of code in there.  Do we have comprehensive ByteArray tests from the player we can add to our test suite?

Should this:

+            return (b << 3) | kIntptrType;


use atomFromIntptrValue?

Some tabs look like they snuck in:

+        ByteArray& GetByteArray() { return m_byteArray; }
+
+		uint32_t get_objectEncoding();
+		void set_objectEncoding(uint32_t version);


And some extra blank lines:

+    void DataIOBase::ThrowEOFError()
+    {
+        toplevel()->throwError(kEndOfFileError);
+
+    }

Comment 37

[Steven Johnson]

(In reply to comment #36)
> Wow, a lot of code in there.  Do we have comprehensive ByteArray tests from the
> player we can add to our test suite?

Good question. I'll check.
 
> use atomFromIntptrValue?

Oh yeah.
 
> Some tabs look like they snuck in:

WIllfix.

Comment 38

[Steven Johnson]

Comment on attachment 481113 [details] [diff] [review]
Chunk #1: Lots of changes from Flash/AIR

pushed as http://hg.mozilla.org/tamarin-redux/rev/a06e4632e581

Comment 39

[Steven Johnson]

Attachment: Chunk #2: errors and byte-swap

More changes from Flash/AIR: 

-- change the error codes thrown for null-argument invalid-argument in a few cases (to match Flash/AIR); this required bringing over new error code and localizations.

-- change the byte-swap macros in DataIO to match those in Flash/AIR (in particular, using MSVC intrinsics); also renamed to avoid a conflict with preprocessor macros in Flash... (sigh)

Comment 40

[Rick Reitmaier]

Comment on attachment 482390 [details] [diff] [review]
Chunk #2: errors and byte-swap

R+ with a few nits:
- error ids 2007 and 2008, why not just use the next values available i.e. 1511, 1512
- Toplevel.h looks like tab issues (?)
- throwArgumentError() has a signature that accepts char* , which is cheaper than creating a String via toErrorString() and then using the Stringp version.

Comment 41

[Steven Johnson]

(In reply to comment #40)
> - error ids 2007 and 2008, why not just use the next values available i.e. 1511, 1512

Because existing Flash acceptance tests look for those specific error numbers. (Otherwise would have left the code as-is)

> - Toplevel.h looks like tab issues (?)

Willfix.

> - throwArgumentError() has a signature that accepts char* , which is cheaper
> than creating a String via toErrorString() and then using the Stringp version.

Thought I caught all those, willfix.

Comment 42

[Steven Johnson]

Comment on attachment 482390 [details] [diff] [review]
Chunk #2: errors and byte-swap

pushed as http://hg.mozilla.org/tamarin-redux/rev/751f973e2c5b

Comment 43

[Steven Johnson]

Attachment: Chunk #3: more errors and MBCS conversion

Still more changes from Flash:

-- synchronizing a few more errors to match the types and values thrown by Flash/AIR; as a result, three flash.error.* Error classes were migrated over, and three more error constants. (Removed kShellCompressedDataError entirely as it's no longer used.)

-- ByteArray.toString() needs to be able to attempt an MBCS conversion in some situations; rather than migrate all the platform-specific code over to Tamarin, I added another hook in Toplevel to allow this to be done by Flash.

-- removed no-longer-needed ThrowMemoryError definition in ByteArray

-- compress2() now uses GetReadableBuffer, not GetWritableBuffer

Comment 44

[Rick Reitmaier]

Comment on attachment 482674 [details] [diff] [review]
Chunk #3: more errors and MBCS conversion

Nothing obviously wrong, although the change from GetWriteBuffer to GetReadBuffer looks odd.

Also, there used to be a script in utils (stale?) that would do some validation on the error constant.  Might be worth running to see if any translations are missing and such.

Comment 45

[Steven Johnson]

(In reply to comment #44)
> Nothing obviously wrong, although the change from GetWriteBuffer to
> GetReadBuffer looks odd.

compress2() is using that as input-only, so it's correct (we never needed a writable buffer)

Comment 46

[Steven Johnson]

Comment on attachment 482674 [details] [diff] [review]
Chunk #3: more errors and MBCS conversion

pushed as http://hg.mozilla.org/tamarin-redux/rev/df2720019f28

Comment 47

[Steven Johnson]

Attachment: Chunk #4: deflate support

Add support for optional args to compress/uncompress. Note that due to unfortunate historical artifact, there is compile-time specialization that the API versioning cannot (yet) deal with, hence the AIR-or-Flash API decision. (avmshell is taking the AIR approach.)

Moved the Compress/Uncompress logic to ByteArray (rather than ByteArrayObject) for more flexibility in use; also allows slightly cleaner implementation.

also added the FP10-versioned inflate/deflate/clear methods.

removed genericzlib.h entirely, as it was a uselessly-thin veneer over zlib.

also some whitespace cleanup.

Comment 48

[Werner Sharp]

Comment on attachment 484069 [details] [diff] [review]
Chunk #4: deflate support

One problem I see:

In the prior implementation, the inflate buffer was left intact at its large size.  A 10 MB inflate call would start with a 10 MB bytearay after copying out the data to a temporary buffer.  The new implementation will reset the buffer to something small (8 KB?) and will be forced to regrow our buffer larger.  It would be better to start the buffer at 10 MB like the prior implementation.

Nits:

Some cut-n-paste tabs made it from the player to the file.  

I was originally worried about the "while (error == Z_OK)" loop being different than the player's code since it did not handle the Z_STREAM_END case specifically but it was discussed via IM.  The only difference is if we get an error code from inflate, we will still call Write() on bogus data (which we throw out in the kCompressedDataError code).

Comment 49

[Steven Johnson]

Attachment: Chunk #4: deflate support (v2)

Added EnsureCapacity call to Uncompress method, fixed tabs (I hope)

Comment 50

[Steven Johnson]

Attachment: Chunk #5: add copy-on-write ownership, initializing

Copy-on-Write ByteArrays need to be able to reference a GC object that owns the data in question, to ensure it isn't collected out from under them. They also need a hook to auto-init themselves based on certain subclass mappings in Flex. This adds those hooks. 

Also removed the copy constructor: it was used in exactly one place, and unnecessarily so as it was just a guard against writing to itself (which has been legal for a long while now).

(Note: this should be the final patch! Once this patch is done, all that's left should be moving various files around, to either core or extensions...)

Comment 51

[Werner Sharp]

Comment on attachment 484106 [details] [diff] [review]
Chunk #4: deflate support (v2)

Looks good.

nit: 

tabs?

+        error = deflateInit2(&stream, 
+                                Z_BEST_COMPRESSION,
+                                Z_DEFLATED, 
+                                algorithm == k_zlib ? MAX_WBITS : MAX_WINDOW_RAW_DEFLATE, 
+                                DEFAULT_MEMORY_USE, 
+                                Z_DEFAULT_STRATEGY);

Comment 52

[Werner Sharp]

Comment on attachment 484123 [details] [diff] [review]
Chunk #5: add copy-on-write ownership, initializing

Looks fine.

What test cases should we add to TR to test all the byte array functionality we are adding into the core code?

Comment 53

[Steven Johnson]

(In reply to comment #51)
> tabs?

Those dang tabs just won't stay away...

(In reply to comment #52)
> What test cases should we add to TR to test all the byte array functionality we
> are adding into the core code?

We need to pull over some of them from ATS.

Comment 54

[Steven Johnson]

Comment on attachment 484106 [details] [diff] [review]
Chunk #4: deflate support (v2)

pushed as http://hg.mozilla.org/tamarin-redux/rev/e88b65fc68ae

Comment 55

[Steven Johnson]

Comment on attachment 484123 [details] [diff] [review]
Chunk #5: add copy-on-write ownership, initializing

pushed as http://hg.mozilla.org/tamarin-redux/rev/e88b65fc68ae

Comment 56

[Steven Johnson]

Attachment: Final patch: move ByteArray, DataIO to core

Comment 57

[Steven Johnson]

Attachment: Post-Final patch

Flash integration showed that adding a byteArrayClass() accessor to Toplevel, and a well-typed constructByteArray() method, would make life much simpler, so here it is.

Comment 58

[Werner Sharp]

Comment on attachment 484588 [details] [diff] [review]
Post-Final patch

tabs:

+        Atom args[1] = { nullObjectAtom };
+		Atom outAtom = construct(0, args);
+		return (ByteArrayObject*)AvmCore::atomToScriptObject(outAtom);

Comment 59

[Werner Sharp]

Comment on attachment 484496 [details] [diff] [review]
Final patch: move ByteArray, DataIO to core

r+ on the assumption that this is moving files around beyond the changes I can see in the diff.  (I did not look at an new file that was just moved from another location).  Please don't include builtin* files in a patch.  They are impossible to review and need regeneration anyway before you submit.

And project files are hard to review.  I assume vc2008 (all configs) and vc2010 all are fixed plus all platforms on the sandbox build server build okay.

Comment 60

[Steven Johnson]

(In reply to comment #59)
> r+ on the assumption that this is moving files around beyond the changes I can
> see in the diff. 

Yep.

> Please don't include builtin* files in a patch.  They are
> impossible to review and need regeneration anyway before you submit.

Normally I exclude 'em, sorry bout that.

> And project files are hard to review.  I assume vc2008 (all configs) and vc2010
> all are fixed plus all platforms on the sandbox build server build okay.

in theory yes. sandbox test not done yet but will be prior to landing.

Comment 61

[Steven Johnson]

pushed to TR as http://hg.mozilla.org/tamarin-redux/rev/df2ee322d110