don't pass / to imageNamed:

[skurfer]

This is responsible for the majority of crashes on Yosemite. Hope this works. I’m going to have a couple of users try it.

The fix is based on a clue in the release notes. I made a small project that threw a bunch of strings at [NSImage imageNamed:] and including a / definitely throws the exception. Couldn’t find any other problematic characters.

[skurfer]

OK, then do we need to change the NSBundle call?

Probably not, but I don’t know what all uses that. (Maybe nothing.)

About point 2:

removing all slashes from all identifiers

You mean in the actual identifier? I.e. in setIdentifier: removing all slashes? That wouldn't work for file objects would it!

Yup. Might only be actions that do that, but still…

[pjrobertson]

Probably not, but I don’t know what all uses that. (Maybe nothing.)

OK, but you're not logging when the slash is removed there because...? It's a category and you can't add an NSMutableSet instance variable?

Once that's answered I think it's OK to go (after I've tested it of course ;-D )

[skurfer]

OK, but you're not logging when the slash is removed there because…?

Mainly because I don’t think it ever will be. I went through the code, and it’s only called from two places. One passes hard-coded strings with no /, and the other follows an existing call to safeImageName:, so it’s already taken care of.

So, I’ve reverted changes in QSFoundation and we handle everything in QSResourceManager. Why not fix it lower down? I’d like Apple to answer that. 😃 But they aren’t fixing it at the lowest levels, so I guess we won’t. And if they ever do, we won’t need to.

Cleaned up this branch and force-pushed.

[pjrobertson]

Cool, in that case I’ll test it and hopefully merge it :)

On 13 Rhag 2014, at 19:19, Rob McBroom notifications@github.com wrote:

OK, but you're not logging when the slash is removed there because…?

Mainly because I don’t think it ever will be. I went through the code, and it’s only called from two places. One passes hard-coded strings with no /, and the other follows an existing call to safeImageName:, so it’s already taken care of.

So, I’ve reverted changes in QSFoundation and we handle everything in QSResourceManager. Why not fix it lower down? I’d like Apple to answer that. But they aren’t fixing it at the lowest levels, so I guess we won’t. And if they ever do, we won’t need to.

—
Reply to this email directly or view it on GitHub.

[pjrobertson]

Hmm... ok, so the plot thickens!

I put a breakpoint in the if() statement you created, and had a look at some of the places that are calling it.
One is an icon in my 1Pwd plugin (/Applications/1Password.app/Contents/Resources/secure-notes-icon-128.png), another is the icon for 'QSPresetVolumes' (it's set as '/')

In -[QSResrouceManager imageNamed:(NSString *)name inBundle:(NSBundle *)bundle] should we first be checking if the name is a path (e.g. by using [name isAbsolutePath] so that we don't hit the disk), and if it is then we can hit the disk and check to see if there's an icon for the given path. If not, continue with what we are doing (stripping away the slash and logging)

[pjrobertson]

@skurfer - not sure if you missed my post here. It's not in the same place as the first, sorry.
See above

[skurfer]

I did, and I’ve been thinking about it. Just haven’t had time to investigate. My gut tells me those examples are wrong twice over: A full path isn’t a valid way to refer to an icon, and imageNamed:inBundle: shouldn’t be called in those cases.

[pjrobertson]

OK, I think you're probably right.

I'd done what I'd done for the 1Password plugin because I wanted to refer to an icon by path in the Info.plist (see here)

Should we therefore change the QSPlugin -[QSPlugIn icon] method (see here) so that it first checks to see if the 'text' is a path and if so try to resolve it? I think it will be an additional extension to the QSPlugin Framework to be able to specify absolute paths in the icon keys.

... or is there another way I should be doing that? QSResourceAdditions? or something?

[skurfer]

In that case, yeah, you could use QSResourceAdditions. There’s an example in the ref.

Apparently, if the image was in your plug-in, you could put the file’s name as the icon. I didn’t know that until looking though the code after your comment.

We could maybe allow it to take a full path, but for now, there’s another way to do it, so it seems low priority.

For the problem at hand, should we just get this out as a band-aid, since it’s causing so many crashes?

[pjrobertson]

OK, yeah. I'm happy with this then. I've added another commit that changes that one path, and have done a quick grep but couldn't find any more icons that use a slash in the name

Edit: So I'm happy for this to be merged and released. If you're happy with my commit, go ahead and merge :)

[pjrobertson]

OK hold on that. Even using QSResoruceAdditions still doesn't seem to work. I get the 'sanitizing image...' message. I'm currently investigating

[pjrobertson]

False alarm - everything is fine. I was just running an older QS version

[skurfer]

If you're happy with my commit, go ahead and merge

Yeah, looks good. I’ll try to merge and release later today. Thanks!

[pjrobertson]

Thanks for putting up with my being picky ;-)

[pjrobertson]

FYI when you're ready to merge/release - we'll need to decide on the QS hosting issue first. (See my QSDev groups post)