Serializing of switch statement/jumps fail Bytecode 1.0RC3

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Deserialize a working flex class with a method that has a case statement in 
it

        override protected function partAdded( partName:String, instance:Object ):void
        {
            super.partAdded( partName, instance );
            switch ( instance )
            {
                case submitButton:
                    submitButton.addEventListener( MouseEvent.CLICK, onSendClicked );
                    break;
            }

        }
2. Serialize the byte array that you get from this 

3. Try to load the class into the AVM2

What is the expected output? What do you see instead?
This should work without problems, but I get errors like the following

[trace]   777:getslot 3
[trace]                        [_ByDi_FlexInit$~[O] 
mx.core::IFlexModuleFactory[O] _ByDi_FlexInit$/init~[O] *[A]] 
{_ByDi_FlexInit$~[O] _ByDi_FlexInit$/init~[O]} (int[I] Array[O])
[trace]   779:getproperty length
[trace]                        [_ByDi_FlexInit$~[O] 
mx.core::IFlexModuleFactory[O] _ByDi_FlexInit$/init~[O] *[A]] 
{_ByDi_FlexInit$~[O] _ByDi_FlexInit$/init~[O]} (int[I] uint[U])
[trace]   781:iflt 0
[Fault] exception, information=VerifyError: Error #1021: At least one branch 
target was not on a valid instruction in the method.
java.lang.IndexOutOfBoundsException: Index: 0, Size: 0
    at java.util.ArrayList.RangeCheck(ArrayList.java:547)
    at java.util.ArrayList.get(ArrayList.java:322)
    at flash.tools.debugger.concrete.DManager.getFrame(DManager.java:669)
    at flash.tools.debugger.concrete.PlayerSession.pullUpActivationObjectVariables(PlayerSession.java:1106)

What version of the product are you using? On what operating system?
1.0.3RC3, OS X 10.7, Flash PLayer 10.3.181.34 (10.3)

Please provide any additional information below.
It seems that there is an issue with serializing some jumps - the worst 
offender appears to be switch statements.

I am trying to fix myself - will let you know how I get on.

Original issue reported on code.google.com by conrad.w...@gmail.com on 30 Jun 2011 at 6:10

[GoogleCodeExporter]

Hey there,

this is a known issue actually, I've stated it in the documentation already. 
You are correct in your observation that the switch statements are the main 
offender.
I have been pouring over this problem for a very long time, but so far haven't 
been able to tackle it. I've contacted Joa Ebert (the guy who builds Apparat) 
about this and he has indicated that the opcode documentation isn't always 
correct. This is probably the case with the swithc statements.
You can observe it by checking out the deserialized opcodes, when examining the 
results for the switch statement in your method you will see that very probably 
the last opcodes in the method body will be a 'lookupswitch', this is of course 
incorrect, since the last opcode *should* be 'returnvoid'.

Clearly something fishy is going on, should you be able to figure out what is 
going wrong I think a LOT of people will be eternally in your debt :)

The first place to check out is the org.as3commons.bytecode.abc.enum.Opcode 
class.

good luck :)

Original comment by ihatelivelyids on 30 Jun 2011 at 7:35

[GoogleCodeExporter]

OK,

I am actually introducing a method to a class at load time. 

Before introducing the method into the class my part added method body looks 
like this

 //maxStack=3, localCount=4, initScopeDepth=12, maxScopeDepth=13
[trace]         debugline       [37]
[trace]         getlocal_0
[trace]         pushscope
[trace]         debug       [1, 20, 0, 37]
[trace]         debug       [1, 21, 1, 37]
[trace]         debugline       [39]
[trace]         getlocal_0
[trace]         getlocal_1
[trace]         getlocal_2
[trace] 
        callsuper       [QName[Namespace[protectedNamespace::spark.components.supportClasse
s:SkinnableComponent]:partAdded], 2]
[trace]         pop
[trace]         jump        [30]
[trace]         label
[trace]         debugline       [49]
[trace]         getlocal_0
[trace]         getproperty     [QName[Namespace[public]:submitButton]]
[trace]         findpropstrict      [QName[Namespace[public::flash.events]:MouseEvent]]
[trace]         getproperty     [QName[Namespace[public::flash.events]:MouseEvent]]
[trace]         getproperty     [QName[Namespace[public]:CLICK]]
[trace]         getlocal_0
[trace] 
        getproperty     [QName[Namespace[private::components:ChatComponent]:onSendClicked
]]
[trace]         callproperty        [QName[Namespace[public]:addEventListener], 2]
[trace]         pop
[trace]         debugline       [50]
[trace]         jump        [51]
[trace]         label
[trace]         jump        [46]
[trace]         debugline       [46]
[trace]         getlocal_2
[trace]         setlocal_3
[trace]         debugline       [48]
[trace]         getlocal_0
[trace]         getproperty     [QName[Namespace[public]:submitButton]]
[trace]         getlocal_3
[trace]         ifstrictne      [6]
[trace]         pushshort       [0]
[trace]         jump        [13]
[trace]         pushfalse
[trace]         iffalse     [6]
[trace]         pushshort       [1]
[trace]         jump        [2]
[trace]         pushshort       [1]
[trace]         kill        [3]
[trace]         lookupswitch        [-40, 1, -65,-40]
[trace]         debugline       [53]
[trace]         returnvoid
[trace]     }
[trace]

After introducing another method the the partAdded method looks likt this

//maxStack=3, localCount=4, initScopeDepth=12, maxScopeDepth=13
[trace]         debugline       [37]
[trace]         getlocal_0
[trace]         pushscope
[trace]         debug       [1, 20, 0, 37]
[trace]         debug       [1, 21, 1, 37]
[trace]         debugline       [39]
[trace]         getlocal_0
[trace]         getlocal_1
[trace]         getlocal_2
[trace] 
        callsuper       [QName[Namespace[protectedNamespace::spark.components.supportClasse
s:SkinnableComponent]:partAdded], 2]
[trace]         pop
[trace]         jump        [30]
[trace]         label
[trace]         debugline       [49]
[trace]         getlocal_0
[trace]         getproperty     [QName[Namespace[public]:submitButton]]
[trace]         findpropstrict      [QName[Namespace[public::flash.events]:MouseEvent]]
[trace]         getproperty     [QName[Namespace[public::flash.events]:MouseEvent]]
[trace]         getproperty     [QName[Namespace[public]:CLICK]]
[trace]         getlocal_0
[trace] 
        getproperty     [QName[Namespace[private::components:ChatComponent]:onSendClicked
]]
[trace]         callproperty        [QName[Namespace[public]:addEventListener], 2]
[trace]         pop
[trace]         debugline       [50]
[trace]         jump        [51]
[trace]         label
[trace]         jump        [46]
[trace]         debugline       [46]
[trace]         getlocal_2
[trace]         setlocal_3
[trace]         debugline       [48]
[trace]         getlocal_0
[trace]         getproperty     [QName[Namespace[public]:submitButton]]
[trace]         getlocal_3
[trace]         ifstrictne      [6]
[trace]         pushshort       [0]
[trace]         jump        [13]
[trace]         pushfalse
[trace]         iffalse     [6]
[trace]         pushshort       [1]
[trace]         jump        [2]
[trace]         pushshort       [1]
[trace]         kill        [3]
[trace]         lookupswitch        [-98, 1, -65,-40]
[trace]         debugline       [53]
[trace]         returnvoid

Which is identical apart from the obviously broken lookupswitch at the end. So 
my theory is that this is a serialisation issue in the bytecode library. If I 
replace the switch statement with an if then the class can be loaded after 
introduction and works fine, which seems to indicate that I am doing the 
introduction correctly - here is what I do

methodInfo.methodBody = methodBody;
methodBody.methodSignature = methodInfo;
method.traitMethod = methodInfo;

instanceInfo.addTrait( method );
abcFile.addMethodBody( methodBody );
abcFile.addMethodInfo( methodInfo );

Is that all the correct linkages for introducing a method?

(Please note that I am not creating a proxy, but introducing a method into a 
class before it is loaded)

Have I got anything wrong?

Original comment by conrad.w...@gmail.com on 30 Jun 2011 at 7:12

[GoogleCodeExporter]

After more investigation:

I think there is some sort of bug in the OpCode.resolveBackPatches method. I 
can see this outputting the -98 when I serialise the instanceInfo. As the 
partAdded method has not changed this number should not change.

BTW here is my process - you can see that this does not involve the Flash 
verifier/player at all

deserialise the swfTag of the class I am interested in
*examine the method body for partAdded (first list above)
introduce method (using the steps I mentioned above)
serialise the abcfile and make that byte array the swftag body
deserialise the new swftag
*examine the method body of partAdded (second list above)

The lookupswitch seems to be correct before resolving back patches, and seems 
to be wrong afterwards. I need to understand more about this back patching 
before ican see what it is doing wrong (if indeed it is)

Original comment by conrad.w...@gmail.com on 1 Jul 2011 at 6:06

[GoogleCodeExporter]

Hi there,

your process is correct indeed. The backpatching works as follows: During 
deserialization all the jump opcodes (ifs, for loops, switches, etc) will have 
their jump targets resolved. Which means using the jump offset the opcode that 
its pointing to is looked up.
Then during serialization all the start and end positions of every opcode are 
saved, afterwards all the jump opcodes are examined again and are checked to 
see if their offsets are still pointing to the correct opcode, if not, the 
correct opcode is looked up they the offsets are 'back patched', which means 
their value is replaced in the serialized bytearray with the correct value.

From your dumps it is quite obvious that the jump targets for switch statements 
are indeed not backpatched properly... :(

By the way, I see you're using a debug version of a SWF, be prepared to run 
into even more weirder errors once you start deserializing/serializing a SWF 
that was compiled as a release build...

Hope that helps a little, good luck!

cheers,

Roland

Original comment by rol...@stackandheap.com on 1 Jul 2011 at 7:11

[GoogleCodeExporter]

If I turn off backpatching everything works as expected :-) This is probably 
because I havent mucked around with the contents of the method body. I'm busy 
today and tomorrow, but will try to have a lok anyway.

Original comment by conrad.w...@gmail.com on 2 Jul 2011 at 6:16

[GoogleCodeExporter]

It seems to be that the targetOpCode for the default jump of a switch statement 
is not properly populated before serialising and so the resolving of the back 
patches miscalculates the jump because of this

var targetOpPos:int = (positions[targetOpcode]) ? positions[targetOpcode] : 0;

which returns 0

This means that the deserialisation of the switch statement is possibly not 
correct.

Thats the next place to look :-)

Original comment by conrad.w...@gmail.com on 2 Jul 2011 at 6:33

[GoogleCodeExporter]

Roland,

I was about to tell you that there are issues with the way all jumps are 
calulated in Opcode and suggest some bug fixes

LOL

I have been using 1.0RC3, and not looking at trunk. If I use trunk none of my 
issues occur and I can see that the bugs I was about to report have already 
been fixed.

So a bit of a red herring this - issues is closed really.

However I did learn a lot about the code :-)

Conrad

Original comment by conrad.w...@gmail.com on 3 Jul 2011 at 8:20

[GoogleCodeExporter]

OK, am now in the situation you mentioned above - 

Debug build with method injection runs fine.
Release build fails with the "At least one branch target was not on a valid 
instruction in the method" error.

If I deserialise the code and reserialise it without introducing the method 
then it all works fine.
If I change the switch statement to an if statement and intriduce the method 
then it works fine.

So it is specifically to do with switch statements and moving the bytecode 
around.

I am still investigating

Original comment by conrad.w...@gmail.com on 3 Jul 2011 at 9:51

[GoogleCodeExporter]

Hi Roland - Please read this one.

I am now running non debug builds using monster debugger for getting 
information. I have found the following issue

Before method introduction the partAdded routine looks like this

(String) = 
    protectedNamespace function QName[Namespace[protectedNamespace::components:ChatComponent]:partAdded](QName[Namespace[public]:String], QName[Namespace[public]:Object]) : QName[Namespace[public]:void]
    {   
        //maxStack=3, localCount=4, initScopeDepth=12, maxScopeDepth=13
        0:getlocal_0        :1
        1:pushscope     :2
        2:getlocal_0        :3
        3:getlocal_1        :4
        4:getlocal_2        :5
        5:callsupervoid     [QName[Namespace[protectedNamespace::spark.components.supportClasses:SkinnableComponent]:partAdded], 2]:9
        9:jump      [25]:13
        13:label        :14
        14:getlocal_0       :15
        15:getproperty      [QName[Namespace[public]:submitButton]]:17
        17:getlex       [QName[Namespace[public::flash.events]:MouseEvent]]:19
        19:getproperty      [QName[Namespace[public]:CLICK]]:22
        22:getlocal_0       :23
        23:getproperty      [QName[Namespace[private::components:ChatComponent]:onSendClicked]]:26
        26:callpropvoid     [QName[Namespace[public]:addEventListener], 2]:30
        30:jump     [46]:34
        34:label        :35
        35:jump     [41]:39
        39:getlocal_2       :40
        40:setlocal_3       :41
        41:getlocal_0       :42
        42:getproperty      [QName[Namespace[public]:submitButton]]:44
        44:getlocal_3       :45
        45:ifstrictne       [6]:49
        49:pushbyte     [0]:51
        51:jump     [12]:55
        55:jump     [6]:59
        59:pushbyte     [1]:61
        61:jump     [2]:65
        65:pushbyte     [1]:67
        67:kill     [3]:69
        69:lookupswitch     [-35, 1, -55,-35]:80
        80:returnvoid       :81
    }

and after method introduction it looks like this

(String) = 
    protectedNamespace function QName[Namespace[protectedNamespace::components:ChatComponent]:partAdded](QName[Namespace[public]:String], QName[Namespace[public]:Object]) : QName[Namespace[public]:void]
    {   
        //maxStack=3, localCount=4, initScopeDepth=12, maxScopeDepth=13
        0:getlocal_0        :1
        1:pushscope     :2
        2:getlocal_0        :3
        3:getlocal_1        :4
        4:getlocal_2        :5
        5:callsupervoid     [QName[Namespace[protectedNamespace::spark.components.supportClasses:SkinnableComponent]:partAdded], 2]:9
        9:jump      [26]:13
        13:label        :14
        14:getlocal_0       :15
        15:getproperty      [QName[Namespace[public]:submitButton]]:17
        17:getlex       [QName[Namespace[public::flash.events]:MouseEvent]]:19
        19:getproperty      [QName[Namespace[public]:CLICK]]:22
        22:getlocal_0       :23
        23:getproperty      [QName[Namespace[private::components:ChatComponent]:onSendClicked]]:26
        26:callpropvoid     [QName[Namespace[public]:addEventListener], 2]:30
        30:jump     [46]:34
        34:label        :35
        35:jump     [41]:39
        39:getlocal_2       :40
        40:setlocal_3       :41
        41:getlocal_0       :42
        42:getproperty      [QName[Namespace[public]:submitButton]]:44
        44:getlocal_3       :45
        45:ifstrictne       [6]:49
        49:pushbyte     [0]:51
        51:jump     [12]:55
        55:jump     [6]:59
        59:pushbyte     [1]:61
        61:jump     [2]:65
        65:pushbyte     [1]:67
        67:kill     [3]:69
        69:lookupswitch     [-35, 1, -55,-35]:80
        80:returnvoid       :81
    }

What we see here is the first jump at line 9 has changed from 25 to 26. This is 
the only difference between the two (according to the debugger). The switch 
statement seems to have survived the method introduction this time.

Original comment by conrad.w...@gmail.com on 3 Jul 2011 at 1:34

[GoogleCodeExporter]

Hey Conrad,

very nice catch, and incredibly weird how that jump gets resolved one bytecode 
position too far... I really hope to have some spare moments in the coming 
weeks to investigate this further. Thanks a lot for your time so far, this is 
indeed a great help!

cheers,

Roland

Original comment by ihatelivelyids on 5 Jul 2011 at 3:07

[GoogleCodeExporter]

Hi Conrad,

I've been busy hacking away on the as3commons-bytecode again. I think I managed 
to get the whole jump business sorted (all changes are in the trunk at the 
moment). Could u send me the SWF that contains your example here? I'd like to 
test it out. You can the file to roland [at] stackandheap dot com.
Thanks a lot!

cheers,

Roland

Original comment by ihatelivelyids on 4 Sep 2011 at 6:49

[GoogleCodeExporter]

ok, skip that, I just ran some more tests but I'm screwing up the switch jumps, 
so its still not working.
I'm not sure if I have any hairs left to pull out of my head though :)

Original comment by ihatelivelyids on 4 Sep 2011 at 7:09

[GoogleCodeExporter]

Well, I believe I have things working properly, sort of. At least my unit test 
that uses a method with a switch statement seems to be passing now, also in 
release mode.

Yet, of course, a new error has reared its ugly head. If I use try/catch blocks 
in a method body everything works fine with a debug build, yet fails with a 
verify error when I use it on a release build...

Original comment by ihatelivelyids on 4 Sep 2011 at 11:45

[GoogleCodeExporter]

Original comment by mastakan...@gmail.com on 11 Sep 2011 at 6:26

• Changed state: Fixed
• Added labels: bytecode

[GoogleCodeExporter]

I'm setting this issue to status 'fixed' as what the title indicates, the 
switch statement parsing is now working.

Original comment by ihatelivelyids on 11 Sep 2011 at 6:38