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

Fixed an annoying crash #342

Merged
merged 3 commits into from
Jun 3, 2011
Merged

Fixed an annoying crash #342

merged 3 commits into from
Jun 3, 2011

Conversation

pjrobertson
Copy link
Member

This one's been bugging me for a while.

I've written an essay here, sorry guys...

I initially thought it was due to my webSearchIcons branch, but it's not. I only just came across it when testing that branch, but have been able to reproduce it in other areas. For example with just simple URLs, and also file objects - I think I came across it when testing Henning's asynchro branch and thought it was related to those commits.

One REALLY easy way to reproduce:

  1. Invoke QS
  2. Type something to get a search URL in the 1st pane (e.g. google )
  3. Hit ',' to add it to your list of objects
  4. Start opening/closing the QS window continuously.

You should get a crash after a while. The reason it's really easy to produce with web search URLs (I think) is because they take more time to resolve (need to get the favicon from online).
I've definitely managed to reproduce it doing the same thing with just URLs.

Now, the reason for the crash:
If you have two or more of the SAME object under the comma trick, QS would try and combine these into a single object. It would then give this object an identifier. In the case of the objects being the same, it would use the identifier of the objects. This would cause QS to go loopy, not knowing which object to access with the identifier.

The fix:
To only combine objects if you have different objects collected using the comma trick.

@pjrobertson
Copy link
Member Author

P.S. I'd been wanting to hold off any releases until this was fixed. I'm happy to release one once this is merged :)

if (!array) [combinedData setObject:(array = [NSMutableArray array]) forKey:type];
[array addObjectsFromArray:[thisObject arrayForType:type]];
// Make sure the object's not already been stored (case when you have two of the same object with the comma trick)
if (![setOfObjects containsObject:thisObject]) {
Copy link
Member

Choose a reason for hiding this comment

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

An NSMutableSet by definition can’t contain duplicates. Is this test necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right you are.
Changed this and pushed

skurfer added a commit that referenced this pull request Jun 3, 2011
@skurfer skurfer merged commit a242b63 into quicksilver:master Jun 3, 2011
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

2 participants