Unify ByteArray between Tamarin and Flash Player

RESOLVED FIXED in Q3 11 - Serrano

Status

Tamarin
Virtual Machine
P3
normal
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: pnkfelix, Assigned: Steven Johnson)

Tracking

(Blocks: 2 bugs)

unspecified
Q3 11 - Serrano
x86
All
Dependency tree / graph
Bug Flags:
in-testsuite ?
flashplayer-qrb +
flashplayer-bug -

Details

(Whiteboard: PACMAN, OOM, sanity)

Attachments

(7 attachments, 8 obsolete attachments)

126.43 KB, patch
Werner Sharp
: review+
Details | Diff | Splinter Review
56.92 KB, patch
Rick Reitmaier
: review+
Details | Diff | Splinter Review
79.67 KB, patch
Rick Reitmaier
: review+
Details | Diff | Splinter Review
139.70 KB, patch
Werner Sharp
: review+
Details | Diff | Splinter Review
17.55 KB, patch
Werner Sharp
: review+
Details | Diff | Splinter Review
480.30 KB, patch
Werner Sharp
: review+
Details | Diff | Splinter Review
5.59 KB, patch
Werner Sharp
: review+
Details | Diff | Splinter Review
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

8 years ago
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.

Updated

8 years ago
Target Milestone: --- → Future

Updated

8 years ago
Flags: flashplayer-qrb+
(Assignee)

Comment 2

8 years ago
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.

Updated

8 years ago
Priority: -- → P3
Summary: ByteArrayGlue.cpp needs updating → Unify ByteArray between Tamarin and Flash Player
Whiteboard: PACMAN, OOM, sanity
Target Milestone: Future → flash10.x - Serrano
(Assignee)

Comment 3

8 years ago
Taking a look at this... one
(Assignee)

Comment 4

8 years ago
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

8 years ago
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.
(Assignee)

Comment 6

8 years ago
Created attachment 479261 [details] [diff] [review]
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)
Assignee: nobody → stejohns
Attachment #479261 - Flags: feedback?(edwsmith)

Comment 7

8 years ago
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.
Attachment #479261 - Flags: feedback?(edwsmith) → feedback+
(Assignee)

Comment 8

8 years ago
Created attachment 479446 [details] [diff] [review]
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)
Attachment #479261 - Attachment is obsolete: true
Attachment #479446 - Flags: review?(edwsmith)

Comment 9

8 years ago
Could we leave the readfile api in ByteArray in avmshell?  conditionally compile it so its not present in flash player builds.
(Assignee)

Comment 10

8 years ago
(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

8 years ago
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.
(Assignee)

Comment 12

8 years ago
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); }
}
(Assignee)

Comment 13

8 years ago
Created attachment 479489 [details] [diff] [review]
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); }
}
Attachment #479446 - Attachment is obsolete: true
Attachment #479489 - Flags: review?(edwsmith)
Attachment #479446 - Flags: review?(edwsmith)
(Assignee)

Updated

8 years ago
Attachment #479489 - Flags: review?(cpeyer)
(Assignee)

Comment 14

8 years ago
Created attachment 479525 [details]
Adding the one binary file

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

Updated

8 years ago
Depends on: 597577

Updated

8 years ago
Blocks: 597577
No longer depends on: 597577

Comment 15

8 years ago
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.
OS: Mac OS X → Windows 7
(Assignee)

Comment 16

8 years ago
(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.
(Assignee)

Comment 17

8 years ago
Created attachment 479644 [details] [diff] [review]
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.
Attachment #479644 - Flags: review?(edwsmith)
(Assignee)

Comment 18

8 years ago
Created attachment 479646 [details] [diff] [review]
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)
Attachment #479646 - Flags: review?(edwsmith)
(Assignee)

Comment 19

8 years ago
Created attachment 479647 [details] [diff] [review]
Patch #4, v1: add IDataInput and IDataOutput

Adds the IDataInput and IDataOutput interfaces (visible as AS3 level).
Attachment #479647 - Flags: review?(edwsmith)
(Assignee)

Comment 20

8 years ago
(Yes, I know tabs/spaces are botched -- assume those will all get fixed before landing)
OS: Windows 7 → Windows XP

Comment 21

8 years ago
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?
(Assignee)

Comment 22

8 years ago
(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

8 years ago
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.
(Assignee)

Comment 24

8 years ago
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

8 years ago
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

8 years ago
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.
(Assignee)

Comment 27

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

review ping...
(Assignee)

Comment 28

8 years ago
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.
Attachment #479644 - Attachment is obsolete: true
Attachment #479644 - Flags: review?(edwsmith)
(Assignee)

Updated

8 years ago
Attachment #479646 - Attachment is obsolete: true
Attachment #479646 - Flags: review?(edwsmith)
(Assignee)

Updated

8 years ago
Attachment #479647 - Attachment is obsolete: true
Attachment #479647 - Flags: review?(edwsmith)
(Assignee)

Comment 29

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

Load-balancing to Werner at Edwin's request
Attachment #479489 - Flags: review?(edwsmith) → review?(wsharp)

Comment 30

8 years ago
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+)
Attachment #479489 - Flags: review?(wsharp) → review+
(Assignee)

Comment 31

8 years ago
(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.

Updated

8 years ago
Attachment #479489 - Flags: review?(cpeyer) → review+

Comment 33

8 years ago
(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?
(Assignee)

Comment 34

8 years ago
Yep.
(Assignee)

Updated

8 years ago
Attachment #479489 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #479525 - Attachment is obsolete: true
(Assignee)

Comment 35

8 years ago
Created attachment 481113 [details] [diff] [review]
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).
Attachment #481113 - Flags: review?(wsharp)

Comment 36

8 years ago
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);
+
+    }
Attachment #481113 - Flags: review?(wsharp) → review+
(Assignee)

Comment 37

8 years ago
(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.
(Assignee)

Comment 38

8 years ago
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
(Assignee)

Comment 39

8 years ago
Created attachment 482390 [details] [diff] [review]
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)
Attachment #482390 - Flags: review?(rreitmai)

Comment 40

8 years ago
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.
Attachment #482390 - Flags: review?(rreitmai) → review+
(Assignee)

Comment 41

8 years ago
(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.
(Assignee)

Comment 42

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

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

Comment 43

8 years ago
Created attachment 482674 [details] [diff] [review]
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
Attachment #482674 - Flags: review?(rreitmai)

Comment 44

8 years ago
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.

Updated

8 years ago
Attachment #482674 - Flags: review?(rreitmai) → review+
(Assignee)

Comment 45

8 years ago
(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)
OS: Windows XP → All
(Assignee)

Comment 46

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

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

Comment 47

8 years ago
Created attachment 484069 [details] [diff] [review]
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.
Attachment #484069 - Flags: review?(wsharp)

Comment 48

8 years ago
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).
Attachment #484069 - Flags: review?(wsharp) → review-
(Assignee)

Comment 49

8 years ago
Created attachment 484106 [details] [diff] [review]
Chunk #4: deflate support (v2)

Added EnsureCapacity call to Uncompress method, fixed tabs (I hope)
Attachment #484069 - Attachment is obsolete: true
Attachment #484106 - Flags: review?(wsharp)
(Assignee)

Comment 50

8 years ago
Created attachment 484123 [details] [diff] [review]
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...)
Attachment #484123 - Flags: review?(wsharp)

Comment 51

8 years ago
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);
Attachment #484106 - Flags: review?(wsharp) → review+

Comment 52

8 years ago
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?
Attachment #484123 - Flags: review?(wsharp) → review+
(Assignee)

Comment 53

8 years ago
(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.
(Assignee)

Comment 54

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

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

Comment 55

8 years ago
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
(Assignee)

Comment 56

8 years ago
Created attachment 484496 [details] [diff] [review]
Final patch: move ByteArray, DataIO to core
Attachment #484496 - Flags: review?(rreitmai)
(Assignee)

Updated

8 years ago
Attachment #484496 - Flags: review?(rreitmai) → review?(wsharp)
(Assignee)

Comment 57

8 years ago
Created attachment 484588 [details] [diff] [review]
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.
Attachment #484588 - Flags: review?(wsharp)

Comment 58

8 years ago
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);
Attachment #484588 - Flags: review?(wsharp) → review+

Comment 59

8 years ago
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.
Attachment #484496 - Flags: review?(wsharp) → review+
(Assignee)

Comment 60

8 years ago
(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.
(Assignee)

Comment 61

8 years ago
pushed to TR as http://hg.mozilla.org/tamarin-redux/rev/df2ee322d110
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

7 years ago
Flags: flashplayer-bug-

Updated

7 years ago
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.