New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[TIMOB-23595] Android: Recompile ti.crypto module against latest SDK #5
Conversation
Could you include the compiled zip/binary as well so that it could be tested easily as well as a sample app.js? It would be very helpful. |
https://github.com/appcelerator-modules/ti.crypto/tree/master/android/example Note that I had to update our usage of Base64 on this module. It was using the apache commons version (which is now gone), and I changed it to the android.util.Base64 version. |
I created a new app and used the new packaged module with latest master build. The app builds and runs normally. But it's giving inconsistent results compared to using version 2.0.2 on Ti SDK 5.3.1. ti.crypto 2.0.2 + ti.sdk 5.3.1.GA |
The example test.js all ran and passed for me:
|
Hmm, you're right, the manual single encrypt/decrypt seems to fail... |
Even weirder, if you keep clicking encrypt, it keeps appending to the encrypted value |
This is driving me insane. It is not very easy to find nice input test data and see if we get what we expect here... So I can't even validate whether one is "right" or not. Clearly the old module at least decrypts back to the original text, so it must be correct. But I don't understand why the changes I made would be the difference here. I'm more concerned that somehow buffer/string translation across the KrollBridge might be a cause. I can't see how the Base64 update would have done it... I suppose I can try and compile with the base64 changes against 5.4.x and verify that way... |
So I recompiled against 5.4.x with the Base64 changes and that worked fine. So... 😢 looks like the V8 update must be the root cause of the issue. Ugh. Need to look into what exactly may have changed here that would cause this. The tests all pass just fine in this module, but obviously the UI based variant fails. So the bug lies somewhere in the difference between the two implementations. |
So something really weird is happening here. I was trying to have Titanium spit out the key/iv used to encrypt to see how it differed. Using 5.4.x/2.0.2, I had it spit out the key at the end of the createKey method and it'd always show as undefined. With 6.0.0/3.0.0, it seemed to hold the last buffer's decrypted text in it already! See old output:
And new:
|
So, apparently it's holding onto the value from last buffer created before it, or something? |
Ok, narrowed it down to a simple test case: var buffer1 = Ti.createBuffer({ value: 'One' }),
buffer2 = Ti.createBuffer({ value: 'Two' }),
buffer3 = Ti.createBuffer({ value: 'Three' }),
buffer4 = Ti.createBuffer({ value: 'Four' });
Ti.API.info(buffer1.value);
Ti.API.info(buffer2.value);
Ti.API.info(buffer3.value);
Ti.API.info(buffer4.value); 6.0.0 prints "Four" four times. 5.4.x prints correctly. I have a theory as to why, I'll see if I can come up with a fix now. |
Ugh, so that's a dead-end. Odd that it behaves differently in that use case, but irrelevant for this, since value is a "normal" property and the module actually calls for the underlying byte[] in Java. Also, printing toString() on each buffer gives us the expected buffer contents. So I guess there's a bug here with the V8 update in that "normal" JS properties are getting shared across instances of the same type (weird). Still trying to figure this one out. Thought it was an issue of aggressive GC moving the JS objects around and our pointers getting stale in C++, but that's not the case. |
Still working on this... The issue of the value being shared across instances is the root cause, as the two text areas stomp over each other and the encryption input value becomes empty, leading to the issue. So I'm trying to debug just where we're falling down in terms of setting the generic JS properties. It's really freaking hard to get useful debug statements logged out for this though... |
Nailed it down, here's the fix: tidev/titanium-sdk#8128 |
CR and FT Passed! APPROVED! |
https://jira.appcelerator.org/browse/TIMOB-23595