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

Webloc crash #2177

Merged
merged 8 commits into from Mar 31, 2016
Merged

Webloc crash #2177

merged 8 commits into from Mar 31, 2016

Conversation

tiennou
Copy link
Member

@tiennou tiennou commented Jan 26, 2016

  • A straightforward fix for #2176.

  • 2020 :

    I'm actually cheating : -pasteboardByFilteringContents: doesn't actually do anything since I-cant-even-begin-to-guess-when so I'm just dispatching something that doesn't work to the main thread so it does nothing later™.

    This code was responsible for extracting contents of .textClipping files (which are stored in resource forks use dark voodoo magic). I know you could use it to right-arrow into text clippings because I played with that looong ago, but I can't seem to get it to work now. I suspect 64bit ? PPC ? I'm also really not sure what it's expecting to get back anyway... @hmelman does that ring a bell to you ?

  • LSCopyItemInfoForURL : the commit says it all. I unfortunately got the crashlog only once. I think I got the explanation right though : the autorelease pool would get drained at the end of a scan "cycle", and since I think it does it's think in another thread, you'd get a crash.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jan 26, 2016

@tiennou - indentation... :S
This is the first test my check_indent script has had.

@@ -600,8 +600,11 @@ + (QSObject *)fileObjectWithPath:(NSString *)path {
// initWithArray: deals with file objects that already exist
QSObject *newObject = [[QSObject alloc] initWithArray:[NSArray arrayWithObject:path]];

if ([clippingTypes containsObject:[[NSFileManager defaultManager] typeOfFile:path]])
[newObject performSelectorOnMainThread:@selector(addContentsOfClipping:) withObject:path waitUntilDone:YES];
if ([clippingTypes containsObject:[[NSFileManager defaultManager] typeOfFile:path]]) {
Copy link
Member

@pjrobertson pjrobertson Jan 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation?!

@tiennou
Copy link
Member Author

@tiennou tiennou commented Jan 26, 2016

Hope I was quick enough that my failed rebase wasn't visible ;-). Looks like Travis saw me though...
I've made some changes to the check_indent script so it's not as painful. It could be better tough but at least you don't end up doing loads of little commits ;-).

I'll add a check for the CFRelease stuff though.

@tiennou
Copy link
Member Author

@tiennou tiennou commented Jan 26, 2016

Yeah, you can also use it locally now, it'll just use master as a "default" diff base. It would be nice if it kept the diff header ;-). And then it would be ready to be promoted to commit-hook status ;-).

In case that wasn't clear I added the tab-switching stuff here (Xcode + TextMate).

@tiennou
Copy link
Member Author

@tiennou tiennou commented Jan 26, 2016

(Also, w.r.t to your lost comment about thread safety, it's actually NDResourceFork that's not threadsafe. I think pasteboardByFilteringContents: is actually fine too...

Anyway, right now this code is dead, I've just kept in because 1) I wasn't sure if it was readily fixable, and 2) I'm not even sure what can be done about NDResourceFork...

@hmelman
Copy link

@hmelman hmelman commented Jan 26, 2016

On Jan 25, 2016, at 8:57 PM, Etienne Samson notifications@github.com wrote:

• This code was responsible for extracting contents of .textClipping files (which are stored in resource forks use dark voodoo magic). I know you could use it to right-arrow into text clippings because I played with that looong ago, but I can't seem to get it to work now. I suspect 64bit ? PPC ? I'm also really not sure what it's expecting to get back anyway... @hmelman does that ring a bell to you ?

Not really sure. The Text Manipulation Plugin allows you to right-arrow into a text file and get back a results list with an item for each line. I haven't used it in all long time. Just trying it now, it seems to work right-arrowing into a .txt file.

Howard

@tiennou
Copy link
Member Author

@tiennou tiennou commented Jan 28, 2016

Yeah, that's right. I think that resource-forky stuff tries to enable the same behavior with clippings. I might be conflating the two then...

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jan 30, 2016

@tiennou - I just cherrypicked your 'smarter script for local deployment' into master.
My first PR since that travis test also just failed because of the same reason :S

Haha! I should really practice what I preach, right? ;-)

tiennou added 8 commits Feb 6, 2016
It's not undeserved because it's `-pasteboardByFilteringContents:` that's not threadsafe ;-).
What happened was that `LSCopyItemInfoForURL` would grab an autoreleased reference to the URL, which would get released by some unrelated pool drain in some other thread, and would end up with a dead URL.
@skurfer skurfer mentioned this pull request Feb 6, 2016
@@ -80,7 +80,7 @@ - (id)initWithPasteboard:(NSPasteboard *)pasteboard {
return [self initWithPasteboard:pasteboard types:nil];
}

- (void)addContentsOfClipping:(NSString *)path { // Not thread safe?
Copy link
Member

@pjrobertson pjrobertson Mar 31, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why remove the comment if it's true?

@pjrobertson pjrobertson merged commit 4696b22 into master Mar 31, 2016
2 checks passed
@pjrobertson pjrobertson deleted the t/webloc-crash branch Mar 31, 2016
skurfer added a commit that referenced this issue Mar 31, 2016
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

Successfully merging this pull request may close these issues.

None yet

3 participants