Skip to content
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

cosscript crash with long running scripts #8

Open
afedor opened this issue Mar 3, 2014 · 42 comments
Open

cosscript crash with long running scripts #8

afedor opened this issue Mar 3, 2014 · 42 comments

Comments

@afedor
Copy link
Contributor

afedor commented Mar 3, 2014

I think this is very similar to issue #5 but I though I'd leave a simple example script that illistrates it for me. It really only appears in 10.9 for me. I've spent a lot of time trying to find the cause but most of the JavaScript stuff is black magic to me. It appears though that due to some garbage collection, javascript objects are disappearing before I'm done using them, but this really only manifests itself when you have a long running javascript script. I even went back and bisected the JavaScriptCore framework to find where the change might have started. I found something around Jun 2012 that when I reverted fixed the problem in most cases (I can give you the exact rev if you want), but that didn't really help with the real problem of finding how I could update CocoaScript to avoid the problem.

Anyway, here's the script. It always seems to crash for me around loop 26, with:
1 0x7fff8d1a0fbc JSC::JSValue::get(JSC::ExecState_, unsigned int, JSC::PropertySlot&) const
2 0x7fff8d1d06fa JSC::getByVal(JSC::ExecState_, JSC::JSValue, JSC::JSValue, JSC::ReturnAddressPtr)
3 0x7fff8d1d034e cti_op_get_by_val_generic
4 0x5971a9c02bd7
5 0x7fff8d1168b6 JSC::Interpreter::execute(JSC::ProgramExecutable_, JSC::ExecState_, JSC::JSObject_)
6 0x7fff8d1156b6 JSC::evaluate(JSC::ExecState_, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*)
7 0x7fff8d1153ad JSEvaluateScript
8 0x106c1a535 -[Mocha evalJSString:scriptPath:]
9 0x106c1a405 -[Mocha evalJSString:]
10 0x106c1a34a -[Mocha evalString:]
11 0x106c3d1fc -[COScript executeString:baseURL:]
12 0x106c3cf8a -[COScript executeString:]
13 0x106c3b1eb main
14 0x7fff969955fd start

==== script ====
var toClass = {}.toString;
var itemDictPath = "/Applications/Xcode.app/Contents/Info.plist";
var itemDict = NSDictionary.dictionaryWithContentsOfFile_(itemDictPath);
var itemList = itemDict["CFBundleDocumentTypes"];

function readDict(item) {
var name = item["CFBundleTypeName"];
var role = item["CFBundleTypeRole"];
print("name class length " + name.length());
//NSLog("role class " + role.class() + " length " + role.length());
}

for (var j = 0; j < 100; j++) {
print(" === LOOP " + j + " ====");
for (var i = 0; i < itemList.count(); i++) {
readDict(itemList[i]);
}
}

@ccgus
Copy link
Owner

ccgus commented Mar 6, 2014

I got it to reproduce as well. Is this on 10.9.2? And I assume you're using the source straight from this repository?

@afedor
Copy link
Contributor Author

afedor commented Mar 6, 2014

Yes. I'm currently running 10.9.2 using the latest pull from the repository.

@samdeane
Copy link
Contributor

I'd be interested in that rev, just in case it suggests anything.

One of my theories is that it's a problem relating to MOBox, which Mocha uses to associate a javascript object with a cocoa object.

These are stored in a map with weak keys but strong values. The key is the cocoa object, so if that goes away, then I think that the MOBox will get dropped from the map.
The problem being that a non-retaining pointer to the box is also stored as the private data for the associated JS object. If that object is still alive, it may attempt to access the MOBox after it has died.

All of which should never happen, as MOBox sets up a retain cycle with the object it represents. If something breaks that cycles somehow though...

@samdeane
Copy link
Contributor

With my hacked together Mocha 2.0 based version, I've managed to get this monster script to crash:

for (var n = 0; n < 10000; n++) {
var test = [NSString stringWithString:"1"];
print(n + ": " + test);
}

@samdeane
Copy link
Contributor

Whereas this doesn't, even with 10 x the iterations:

for (var n = 0; n < 100000; n++) {
var test = [NSString stringWithString:@"1"];
print(n + ": " + test);
}

@afedor
Copy link
Contributor Author

afedor commented Sep 26, 2014

Well the actual revision where the break occurs is SVN r120897 of WebKit, but note that if I revert that particular revision it fixes the problem with the example script I gave above, but it does not fix it for other (more complicated) examples. Also note that it breaks both jstalk and Cocoascript, but I was just focusing on Cocoascript as it's being maintained. Here's the actual patch I made to try to revert it, in case you don't want to spend several hours or so checking out webkit, etc...

Index: runtime/JSObject.cpp
===================================================================
--- runtime/JSObject.cpp    (revision 151722)
+++ runtime/JSObject.cpp    (working copy)
@@ -57,12 +57,16 @@

 JSCell* getCallableObjectSlow(JSCell* cell)
 {
+#if 1
+    return getJSFunction(cell);
+#else
     Structure* structure = cell->structure();
     if (structure->typeInfo().type() == JSFunctionType)
         return cell;
     if (structure->classInfo()->isSubClassOf(&InternalFunction::s_info))
         return cell;
     return 0;
+#endif
 }

@samdeane
Copy link
Contributor

My suspicion is that it's some sort of memory overwrite coming from Mocha, in which case the WebKit revision might be a coincidence. If the memory access pattern changes slightly, it could mask the crash again for any given case of it.

@afedor
Copy link
Contributor Author

afedor commented Jan 24, 2015

I currently believe this is more an issue with JavaScriptCore than with CS (or Mocha), so you can probably close this if you like. I've filed an Apple rdar://19378158 (http://openradar.appspot.com/radar?id=6174800997253120). I also feel this is similar or perhaps the same bug reported here (https://bugs.webkit.org/show_bug.cgi?id=131682), which is just raw JavaScriptCore functions.

@lukasondre
Copy link

Any updates on this issue? I'm using CocoaScript to write Sketch plugins and I keep getting cti_op_call_NotJSFunction crashes. Even just iterating over a simple array with several 1000s elements crashes it.

Interestingly, if I put in a delay of about a second after each few 100s, it doesn't crash! But that makes execution way too slow and unreliable.

Please let me know how I can help and what the progress with fixing this major issue is.

@afedor
Copy link
Contributor Author

afedor commented Feb 6, 2015

No response from the Apple or WebKit guys. I still believe that JSC is garbage collecting some object when it shouldn't be, but I could spend hours single-stepping through the JSC framework and not get anywhere. I'll keep trying though.

@ccgus
Copy link
Owner

ccgus commented Feb 6, 2015

I'm wondering if a similar crash could be reproduce with JavaScript for Automation- and that's another way to get a real radar filed. I've not been able to reproduce it in Script Editor though.

@afedor
Copy link
Contributor Author

afedor commented Apr 2, 2015

In case anyone is interested, I have a fix for this in my fork https://github.com/afedor/CocoaScript master branch. It uses the dreaded JSValueProtect/Unprotect, so it's a bit of a hack, but it works really well. I've been using it in production code with many scripts some running several hundred lines long.

@samdeane
Copy link
Contributor

samdeane commented Apr 2, 2015

It's a nice pragmatic workaround...

@ccgus
Copy link
Owner

ccgus commented Apr 3, 2015

And thus it was merged. We'll see how this goes :)

@afedor
Copy link
Contributor Author

afedor commented Apr 3, 2015

FYI #24 is not related to this bug, although it does help a bit.

@samdeane
Copy link
Contributor

samdeane commented Apr 7, 2015

It looks like we might have an alternative fix here: https://github.com/logancollins/Mocha/pull/23.

The Mocha code in the main branch of Cocoascript is a copied-in version of Mocha 1.0 (with a few modifications).

I have a branch which is re-worked to pull in Mocha 2.0 as a submodule. We should be able switch over to using that (for Sketch, at least), which means we can pull in that Mocha fix directly.

@ccgus
Copy link
Owner

ccgus commented Apr 7, 2015

Sounds good, where's the branch? I can merge that guy in (or if you want to do a pull request, that's super easy as well).

@afedor
Copy link
Contributor Author

afedor commented Apr 7, 2015

This is actually just a smarter implementation of my fix #24 (or logaincollins/Mocha#21), which, while it does help some things, it doesn't actually fix the crashes, either on the master or 2.0 branch of Mocha

@afedor
Copy link
Contributor Author

afedor commented Apr 7, 2015

I could provide a pull request that reverts my change and uses this one if you want. It's currently on the same branch as my previous pull

@ccgus
Copy link
Owner

ccgus commented Jul 6, 2015

Hello.

So, I've been trying to get Cocoa Script (mainline) to crash using some simple examples, and can't seem to do it anymore on 10.10.4. Anyone got an example that still does it? I'm also trying out the address sanitizer, to see if that finds anything.

@ccgus
Copy link
Owner

ccgus commented Jul 6, 2015

wooooooo:

=================================================================
==47764==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fff8a06c064 bp 0x7fff5d10f3f0 sp 0x7fff5d10f3d8 T0)
    #0 0x7fff8a06c063 in objc_retain (/usr/lib/libobjc.A.dylib+0x9063)
    #1 0x1051a902d in MOObject_finalize (/builds/acorn-cduspeibhaptzecrrqdrotwjeezx/Build/Products/Debug/CocoaScript.framework/Versions/A/CocoaScript+0x4802d)
    #2 0x7fff8c282226 in JSC::JSCallbackObjectData::finalize(JSC::Handle<JSC::Unknown>, void*) (/System/Library/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x145226)
    #3 0x7fff8c177217 in JSC::WeakBlock::sweep() (/System/Library/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x3a217)
    #4 0x7fff8c156617 in JSC::WeakSet::sweep() (/System/Library/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x19617)
    #5 0x7fff8c1565a8 in JSC::MarkedBlock::sweep(JSC::MarkedBlock::SweepMode) (/System/Library/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x195a8)
    #6 0x7fff8c564edd in JSC::MarkedAllocator::tryAllocateHelper(unsigned long) (/System/Library/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x427edd)
    #7 0x7fff8c15582b in JSC::MarkedAllocator::allocateSlowCase(unsigned long) (/System/Library/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x1882b)
    #8 0x7fff8c1a9040 in JSC::JSCallbackObject<JSC::JSDestructibleObject>::create(JSC::ExecState*, JSC::JSGlobalObject*, JSC::Structure*, OpaqueJSClass*, void*) (/System/Library/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x6c040)
    #9 0x7fff8c1a8e0f in JSObjectMake (/System/Library/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x6be0f)
    #10 0x1051b7e3e in -[Mocha boxedJSObjectForObject:] (/builds/acorn-cduspeibhaptzecrrqdrotwjeezx/Build/Products/Debug/CocoaScript.framework/Versions/A/CocoaScript+0x56e3e)
    #11 0x1051b72a4 in -[Mocha JSValueForObject:] (/builds/acorn-cduspeibhaptzecrrqdrotwjeezx/Build/Products/Debug/CocoaScript.framework/Versions/A/CocoaScript+0x562a4)
    #12 0x1051ad31a in MOBoxedObject_getProperty (/builds/acorn-cduspeibhaptzecrrqdrotwjeezx/Build/Products/Debug/CocoaScript.framework/Versions/A/CocoaScript+0x4c31a)
    #13 0x7fff8c4fa302 in JSC::JSCallbackObject<JSC::JSDestructibleObject>::callbackGetter(JSC::ExecState*, JSC::JSObject*, long long, JSC::PropertyName) (/System/Library/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x3bd302)
    #14 0x7fff8c3538bd in llint_slow_path_get_by_id (/System/Library/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x2168bd)
    #15 0x7fff8c558dea in llint_entry (/System/Library/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x41bdea)
    #16 0x7fff8c556490 in callToJavaScript (/System/Library/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x419490)
    #17 0x7fff8c4da5e2 in JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) (/System/Library/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x39d5e2)
    #18 0x7fff8c17dd7b in JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*) (/System/Library/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x40d7b)
    #19 0x7fff8c17b943 in JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*) (/System/Library/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x3e943)
    #20 0x7fff8c17b669 in JSEvaluateScript (/System/Library/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x3e669)
    #21 0x1051baaa1 in -[Mocha evalJSString:scriptPath:] (/builds/acorn-cduspeibhaptzecrrqdrotwjeezx/Build/Products/Debug/CocoaScript.framework/Versions/A/CocoaScript+0x59aa1)
    #22 0x1051ba2f3 in -[Mocha evalString:atURL:] (/builds/acorn-cduspeibhaptzecrrqdrotwjeezx/Build/Products/Debug/CocoaScript.framework/Versions/A/CocoaScript+0x592f3)
    #23 0x1051d334d in -[COScript executeString:baseURL:] (/builds/acorn-cduspeibhaptzecrrqdrotwjeezx/Build/Products/Debug/CocoaScript.framework/Versions/A/CocoaScript+0x7234d)
    #24 0x1051d295e in -[COScript executeString:] (/builds/acorn-cduspeibhaptzecrrqdrotwjeezx/Build/Products/Debug/CocoaScript.framework/Versions/A/CocoaScript+0x7195e)
    #25 0x102b6300c in -[TSApplication scriptingDoJavaScript:] (/builds/acorn-cduspeibhaptzecrrqdrotwjeezx/Build/Products/Debug/Acorn.app/Contents/MacOS/Acorn+0x10007700c)```

@afedor
Copy link
Contributor Author

afedor commented Jul 6, 2015

I have some Unit Tests written for the Mocha framework that illustrate it. I could adapt them for CS

@ccgus
Copy link
Owner

ccgus commented Jul 6, 2015

I'd love to see one, danke.

@ccgus
Copy link
Owner

ccgus commented Jul 6, 2015

Some notes to myself here.

If I change a script from
newDoc.dataOfType("public.png").writeToFile("/tmp/foo.png")

to:

var data = newDoc.dataOfType("public.png")
data.writeToFile("/tmp/foo.png")

Then I don't get a crasher from an overrelease of an NSData object.

@samdeane
Copy link
Contributor

samdeane commented Jul 7, 2015

Yay for address sanitizer - seems like just what we need here.

@ccgus
Copy link
Owner

ccgus commented Jul 7, 2015

Now if I can just get JSCore to build…

@afedor
Copy link
Contributor Author

afedor commented Jul 7, 2015

If you're thinking of build JSCore from WebKit, it's easiest to just checkout the package from svn:

svn co https://svn.webkit.org/repository/webkit/trunk

and build all of webkit (takes a long time)

./Tools/Scripts/build-webkit --debug

@ccgus
Copy link
Owner

ccgus commented Jul 7, 2015

I'll try that out this evening, thanks.

@ccgus
Copy link
Owner

ccgus commented Jul 8, 2015

Well… I couldn't get JSCore to build with the sanitizer. Boo.

@afedor
Copy link
Contributor Author

afedor commented Jul 9, 2015

Well if it helps, I don't think this is really a memory issue. It's really sort-of expected behavior. JSCore can garbage collect values at any time if it doesn't think they are being used internally or on the stack. What I can't figure out is that Mocha is supposed to get a Finalize callback when the objects are garbage collected so it can remove them from it's own reference, but that doesn't happen.

@samdeane
Copy link
Contributor

samdeane commented Jul 9, 2015

Yeah, I still sort of suspect that it's a memory overwrite, which is perhaps causing JSCore to somehow fail to send that callback, maybe caused by the ffi stuff since that's the only real code which is messing about with memory at a lower level.

I have no real evidence for this though, it's just a feeling in my waters :)

@ccgus
Copy link
Owner

ccgus commented Jul 9, 2015

I'm trying to track down two bugs. The first one is the one that @afedor has with his unit tests- this I believe is a JSCore bug.
The other one that I've got is an overrelease of various bridged objects. I've been using Instruments to track it down, and I know exactly when everything is about to crash- but I can't seem to figure out exactly where it's happening. Turns out when you use ARC, it's super hard to know exactly what's going on.

Anyway, spent a couple of hours yesterday doing that. I think Mocha is doing a bit too much retaining and releasing, so I'm trying to simplify that as well.

@ccgus
Copy link
Owner

ccgus commented Jul 10, 2015

Alright, now I see why distributed objects are evil.

I've found a bug, or I guess you could say a problem, with the DO stuff. NSArrays an NSData objects are over released for some reason. I'm wondering if they are special cased somehow with DO, and copies are sent over the wire to the local address space, and it's just not getting taken care of correctly. I'll play with this some more later.

@samdeane
Copy link
Contributor

Something just came up with Sketch, which might be relevant to this discussion.

We have a crash with a script that accesses NSColorSpace.genericRGBColorSpace.

@randomsequence was looking into it, and tracked down these documentation references:

https://www.mikeash.com/pyblog/friday-qa-2011-09-30-automatic-reference-counting.html

ARC's implementation of zeroing weak references requires close coordination between the Objective-C reference counting system and the zeroing weak reference system. This means that any class which overrides retain and release can't be the target of a zeroing weak reference. While this is uncommon, some Cocoa classes, like NSWindow, suffer from this limitation. Fortunately, if you hit one of these cases, you will know it immediately, as your program will crash with a message like this:

objc[2478]: cannot form weak reference to instance (0x10360f000) of class NSWindow

and

https://developer.apple.com/library/ios/#releasenotes/ObjectiveC/RN-TransitioningToARC/_index.html

Which classes don’t support zeroing-weak references?

You cannot currently create zeroing-weak references to instances of the following classes:

NSATSTypesetter, NSColorSpace, NSFont, NSFontManager, NSFontPanel, NSImage, NSMenuView, NSParagraphStyle, NSSimpleHorizontalTypesetter, NSTableCellView, NSTextView, NSViewController, NSWindow, and NSWindowController. In addition, in OS X no classes in the AV Foundation framework support weak references.

Via [http://stackoverflow.com/q/9146540/272473](Stack Overflow)

@samdeane
Copy link
Contributor

He pointed out that Mocha is using an NSMapTable for the boxing, but the NSMapTable documentation has this comment:

Use of weak-to-strong map tables is not recommended. The strong values for weak keys which get zeroed out continue to be maintained until the map table resizes itself.

@samdeane
Copy link
Contributor

It’s been a while since I looked at the Mocha implementation, so I can’t recall if this is still the case, but one or both of these points might be relevant to some of the script crashes we’ve seen.

@ccgus
Copy link
Owner

ccgus commented Aug 19, 2015

Here's what mocha does:

_objectsToBoxes = [NSMapTable mapTableWithKeyOptions:NSMapTableWeakMemory | NSMapTableObjectPointerPersonality valueOptions:NSMapTableStrongMemory | NSMapTableObjectPointerPersonality]

So, it sounds like this might be a problem.

I don't think the zeroing weak ref is a problem though- I'm pretty sure we would have hit it already. But, I'm happy to be proven wrong.

I wonder if there's something else we could drop in for the _objectsToBoxes ivar?

@afedor
Copy link
Contributor Author

afedor commented Aug 20, 2015

There doesn't seem to be any harm in making the keys strong just for testing (just excess memory usage). It still fails some of my UnitTests with that change. But perhaps there's something to that line of thinking.

@tarngerine
Copy link

any updates on this front? running into a sketch plugin crash with NSFont (but only on 3.4 beta, 3.3.3 works fine) cc @samdeane

@afedor
Copy link
Contributor Author

afedor commented Sep 18, 2015

I'm not sure it's the same issue if it works in one version but not another. You can probably tell if you change the script slightly and the crash happens in a different place or not at all.

Anyway, I have a hack that fixes it (See my Apr 2 comment), but this has lead me to be lazy about finding the real problem.

@samdeane
Copy link
Contributor

Hmm... 3.4 beta definitely shouldn't have made things worse, so that might be a separate issue.

Could you report that one to mail@bohemiancoding.com?

@tarngerine
Copy link

Just sent an email!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants