search

ccgus / CocoaScript

JavaScript + the Cocoa frameworks, and then ObjC brackets show up to party as well.
Other
618 stars 58 forks source link

People/sam/cocoascript boxing cleanup #40

Open samdeane opened 8 years ago

samdeane commented 8 years ago

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 JSValueProtect on 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:

I can then balance these with calls to JSValueUnprotect:

I'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 JSValueProtect everywhere, 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...

ccgus commented 8 years ago

Finally getting around to this. Why main thread for MOBoxManager initWithContext? Just to make things simpler?

samdeane commented 8 years ago

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.

samdeane commented 8 years ago

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.

samdeane commented 8 years ago

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.