More crashes #986

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
4 participants
Owner

pjrobertson commented Jul 11, 2012

We're still getting QSLibrarian/QSCatalogEntry crashes related to multi threading with 3930. See the commit notes for details of these changes, which I hope will be the final fixes!

pjrobertson added some commits Jun 26, 2012

@pjrobertson pjrobertson Merge branch 'release' of github.com:quicksilver/Quicksilver into rel…
…ease
784cd90
@pjrobertson pjrobertson Merge branch 'release' of github.com:quicksilver/Quicksilver into rel…
…ease
160445a
@pjrobertson pjrobertson Merge branch 'release' of github.com:quicksilver/Quicksilver into rel…
…ease
e0f5043
@pjrobertson pjrobertson Merge branch 'release' of github.com:quicksilver/Quicksilver into rel…
…ease
2f91440
@pjrobertson pjrobertson Merge branch 'release' of github.com:quicksilver/Quicksilver into rel…
…ease
4d38005
@pjrobertson pjrobertson Attempt to fix more multi-threading crashes
In QSCatalogEntry, calling [self contents] was itself calling `saveIndex`... causing recursive calls. Also, the `copy` was a vain attempt at fixing the problem. It only performs a shallow copy, leaving the NSArray's contents (NSMutableDicts to be precise) free to be mutated.
The QSLibrarian Fix ensures that catalog contents cannot be altered whilst they're being added to.
95f510e
@pjrobertson pjrobertson Get `[self _contents]` directly b88b21e
Owner

HenningJ commented Jul 12, 2012

May it's enough, to copy data and meta in QSObject_PropertyList dictionaryRepresentation. I think that should be enough.
But maybe we actually need a deep copy. I'm not sure. :-/

Owner

pjrobertson commented Jul 12, 2012

yeah, what if the meta and data dicts themselves refer to other mutable
dicts and mutable arrays? It's probably unlikely, but could be the case I
guess.

Apple themselves seem to recommend deepCopy in certain situations. See the
deep copy section in:
https://developer.apple.com/library/mac/#documentation/CoreFOundation/Conceptual/CFMemoryMgmt/Concepts/CopyFunctions.html#//apple_ref/doc/uid/20001149-CJBEJBHH
The only thing is, it's pretty wasteful. If we copied the data and meta
bits in dictionaryRepresentation, would they be released, or would we
need to do [[data copy] autorelease]

On 12 July 2012 10:42, Henning Jungkurth <
reply@reply.github.com

wrote:

May it's enough, to copy data and meta in QSObject_PropertyList
dictionaryRepresentation
.
I think that should be enough.
But maybe we actually need a deep copy. I'm not sure. :-/


Reply to this email directly or view it on GitHub:
#986 (comment)

Owner

HenningJ commented Jul 12, 2012

yeah, what if the meta and data dicts themselves refer to other mutable dicts and mutable arrays? It's probably unlikely, but could be the case I guess.

I think it's unlikely. I'm not sure if it's even possible.
But you're right, we should probably be sure and deepcopy it in saveIndex. But what happens if it's modified during the copy? I guess it's just as likely as being modified during saving (ie. not very likely, but obviously possible).

I guess the more correct approach would be to @synchronize all modifications of QSObjects. Sounds even more expensive. :-/

Owner

pjrobertson commented Jul 12, 2012

Yeah, all this is expensive. But in the fight for stability, they should be
worth it.
I guess we should really look at where the different calls are being made
and try to stop the multithreading issue from the ground up. We could
possible use some GCD to run it on a certain thread, and maybe block other
threads from doing stuff?

On 12 July 2012 11:06, Henning Jungkurth <
reply@reply.github.com

wrote:

yeah, what if the meta and data dicts themselves refer to other mutable
dicts and mutable arrays? It's probably unlikely, but could be the case I
guess.

I think it's unlikely. I'm not sure if it's even possible.
But you're right, we should probably be sure and deepcopy it in
saveIndex. But what happens if it's modified during the copy? I guess
it's just as likely as being modified during saving (ie. not very likely,
but obviously possible).

I guess the more correct approach would be to @synchronize all
modifications of QSObjects. Sounds even more expensive. :-/


Reply to this email directly or view it on GitHub:
#986 (comment)

Owner

pjrobertson commented Jul 12, 2012

I just thought I'd add some comments from @tiennou so the discussion can all stay in one place:

From Etienne:

Hmm… Hard really ;-). I guess the easy thing would be to deepCopy stuff around, but that will make memory balloon for a little while each time the catalog gets written to disk.

The other solution I see is move the writing stuff out of the scanning process, and write when you get a QSCatalogEntryIndexed notification, or when the indexing is done at the QSLibrarian level. I'm not sure that will do the trick, as some sources are likely to tinker with their contents before you get the chance to write it out, leading to the same problem.

Or another one, write the content array before setting it as the current content array (because around L509 you have the only known pointer) ;-).

I'm also wondering at the test at https://github.com/pjrobertson/Quicksilver/blob/e7ee335629cc720181d7709385df67a7d7b9a3cb/Quicksilver/Code-QuickStepCore/QSCatalogEntry.m#L513 . Does it really mean "If that source can't be indexed (doesn't respond to -entryCanBeIndexed:) or it can be indexed" then save the index ? And there's a -canBeIndexed message above that does exactly the same thing.

Owner

tiennou commented Jul 12, 2012

Yeah, after giving that a few more thinking I guess hacking -setContents: so that it triggers a save before setting the ivar would fix the issue.

Owner

tiennou commented Jul 12, 2012

We could possible use some GCD to run it on a certain thread, and maybe block other
threads from doing stuff?

And yes, I was thinking that having an NSOperationQueue managed by QSLibrarian should make multithreading stuff more simple, since it would ensure that the queue is either 1) doing nothing ;-), 2) scanning an item, or 3) writing the catalog, instead of the current mutability weirdness.

Owner

pjrobertson commented Jul 12, 2012

But if you look at the scanAndCache method (in QSCatalogEntry.m) this is
effectively what happens, although in a different order. setContents is
first called, (line 511) then saveIndex (line 514).

The problem seems to be that there are mutable dictionaries inside the
'contents' which are getting altered somewhere, but we don't really know
where. I guess the right thing to do is to never add mutable objects to the
array in any 'objectsForEntry' method, to avoid them from being mutated.
If we looked through all the plugins, we could probably find which
'objectsForEntry' method does this, and pinpoint it as the cause of the
problem.

Thinking it over again, I think Henning might be on the right track, with
it being the data and meta iVars (both mutable dicts) that are the
things being altered when writeArray is being written to disk. So perhaps
the solution is to copy these inside the dictionaryRepresentation
method in QSObject.m

On 12 July 2012 20:31, Etienne Samson <
reply@reply.github.com

wrote:

Yeah, after giving that a few more thinking I guess hacking
-setContents: so that it triggers a save before setting the ivar would
fix the issue.


Reply to this email directly or view it on GitHub:
#986 (comment)

Owner

skurfer commented Jul 12, 2012

So perhaps the solution is to copy these inside the dictionaryRepresentation method in QSObject.m

Oh, hey, as long as you’re poking around in that method, fix #555. ;-)

Owner

tiennou commented Jul 12, 2012

Hem, you lost me here ;-). -objectsForEntry: returns an (immutable) NSArray of (mutable) QSObjects, so the problem is likely in one of those QSObjects. It means there's no point (IMHO) in looking at each plugin since they all do the same thing (build objects, pack them in array, and return).
Okay, I see how Henning might be on the right track now ;-) with something that changes either the meta or the data dict (my money's on meta, especially since I think it gets an update when an object appears in the interface). And yeah, the only way to fix that is to -copy stuff before stuffing 'em in -dictionaryRepresentation and check for NSCopying exceptions...

Owner

HenningJ commented Jul 13, 2012

Okay, I see how Henning might be on the right track now ;-) with something that changes either the meta or the data dict (my money's on meta, especially since I think it gets an update when an object appears in the interface). And yeah, the only way to fix that is to -copy stuff before stuffing 'em in -dictionaryRepresentation and check for NSCopying exceptions...

Yeah, that's what I thought. But even when they are copied, their elements can still point to the same objects as their uncopied siblings. And those objects might still be modified, right? Less likely, but still possible.

Owner

tiennou commented Jul 13, 2012

Hmm, I think the exception is raised because of a dictionary getting modified, not because a value in one of the dictionary changed, else you'd be getting something like "incompatible object for property list serialization".

Owner

pjrobertson commented Jul 13, 2012

Yeah, it's a fast enumeration exception. I.e. the dict was mutated whilst
being enumerated. i.e. a new key/value being added or removed, or
setObject: forMeta: was being called.

Unless an object's meta (or data) contains a mutable dict or array (highly
unlikely) then Henning's fix will work. Of course, there's always a risk
that a mutable dict/array might be set, but I've just checked all the
setObject:forMeta methods and none do set mutable dits/arrays. Just
strings and URLs mainly.

On 13 July 2012 15:23, Etienne Samson <
reply@reply.github.com

wrote:

Hmm, I think the exception is raised because of a dictionary getting
modified, not because a value in one of the dictionary changed, else you'd
be getting something like "incompatible object for property list
serialization".


Reply to this email directly or view it on GitHub:
#986 (comment)

Owner

HenningJ commented Jul 13, 2012

Unless an object's meta (or data) contains a mutable dict or array

That was my point. But if that doesn't happen, great. One less thing to worry about.

Owner

skurfer commented Jul 23, 2012

This didn’t make it prior to the B69 release, so it should probably be changed to go against master.

pjrobertson referenced this pull request Jul 24, 2012

Merged

Crash Fixes #1015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment