People/sam/cocoascript boxing cleanup #40
Conversation
Unfortunately there's no other way to suppress them optionally that I can find, and they were the only warnings in our build.
Import paths can be in the format:
relative/to/script.js
/absolute/path/to/script.js
~/relative/to/home/script.js
BohemianCoding/Sketch#3319 Script import: Add support for absolute and ~ paths
Also guards against multiple imports of the same URL, and therefore circular imports Ref BohemianCoding/Sketch#3319
BohemianCoding/Sketch#3319 Add support for nested @import statements
Fix a bug which ignored first line of @import-ed scripts
…tom class instead of a dictionary.
|
Finally getting around to this. |
|
Actually, I don't think the thread matters per-se, I was just trying to check that the box manager wasn't being hit from multiple threads. Probably it should record the thread that it was initialised on, and just assert that it's only used from that thread. Could also make it thread-safe of course, but I was wondering if it was some sort of race condition causing the crash, so the checks were just a little bit of investigation. |
…cle which would otherwise occur if the runtime contains a global object that owns a reference to the COScript object (which happens in Sketch).
…ocoascript-boxing-cleanup # Conflicts: # Cocoa Script.xcodeproj/project.pbxproj # src/framework/COScript.m
… the boxed object, so that we can annotate it with extra debug if necessary.
We were sometimes missing out some boxes when performing cleanup, because the keys returned to us by (for NSValue* key in _index) were failing to return a value when [_index objectForKey:key] was called! I presume that this is because the key lookup was using isEqual: and somehow was failing to match somehow. To avoid this we turn on NSMapTableObjectPointerPersonality for the keys. For extra paranoia, we iterate the collection's objects, rather than its keys, so that we can be certain we're getting them all.
|
I've been looking at this yet again, in relation to another problem we had. Our crude workaround for the crash (not this branch, but in the branch that Sketch is currently using) was just to leak all the JS objects until we cleaned up the context after the script executed. However at some point we created a retain cycle so we weren't actually cleaning up the context ever - not good. In fixing that I uncovered another problem with cleanup, so I thought I'd give this branch another try whilst debugging it. The cleanup problem still happened on this branch, and I tracked it down to an issue with the lookup table that keeps the boxed objects, which was failing to look up the box sometimes because it wasn't using pointer identity for the keys. |
|
That pointer identity fix was something that Logan has actually incorporated into Mocha now, but when I rewrote the boxing stuff in this branch I missed it. I'm still not sure if this branch will actually fix the main problem we're seeing, but I'll let you know. |
I'm not sure if you'll want to take everything here, as there might be some Sketch specific things (I'm not certain of that, but I'm aware that I haven't had a chance to factor out all the changes nicely). There are certainly some project changes caused by upgrading to XC7/8, for example, and some other older debugging changes.
However, you might want to take an attempt I made to clean up the boxing, and a possible fix for the Mocha crash.
We had been operating with the "fix" that just called
JSValueProtecton every object, and thus leaked all the JS objects.I had yet another think about what the problem might be (assuming that it's not a bug in JSCore), and the theory I came up with was that either:
I suspect that the second situation might be happening with
MOStruct. We seem to make JS objects here, and thus cause them to be boxed, but we don't actually hold on to the JS refs anywhere that I can see, other than the boxes themselves. The objects seem to be made in passing, as a way of extracting structures out of ffi return values.My speculation is that in this situation JSCore might be "smart" enough not to bother actually initialising/finalising the object at all (or it might be a bug that it doesn't get finalised).
My fix for these two possible scenarios is to call
JSValueProtect:MOStructI can then balance these with calls to
JSValueUnprotect:MOStructis destroyedI'm not certain that this is a valid fix, but it seems to work in one of the cases that was crashing for me here.
It's a slightly better fix than just calling
JSValueProtecteverywhere, as it does allow some objects to be freed up.I've also improved the cleanup that COScript/MochaRuntime does when shutting down. In particular, they clear out the box table, and the script removes its reference to the runtime, thus breaking a retain cycle that was happening for us (because, I think, the runtime contained a JS variable that had a reference to the script).
Note: This is a revision of #39, which was a revision of #34. I'm doing my best to confuse everyone...