xpra icon
Bug tracker and wiki

Opened 4 weeks ago

Closed 4 weeks ago

Last modified 4 weeks ago

#1750 closed defect (fixed)

Segfault on mac os 10.13.2

Reported by: unkulunkulu Owned by: unkulunkulu
Priority: major Milestone:
Component: android Version: 2.2.x
Keywords: Cc:

Description (last modified by Antoine Martin)

xpra v2.2.1-r17715 reproducibly segfaults on mac os high sierra 10.13.2

The problem seems to be near this line http://xpra.org/trac/browser/xpra/trunk/src/xpra/platform/darwin/keyboard.py#L102

At least this simple script sefaults 100% of time:

from AppKit import NSTextInputContext
ic = NSTextInputContext.new()
print ic
del ic

At first I reported it to pyobjc: https://bitbucket.org/ronaldoussoren/pyobjc/issues/235/nstextinputcontext-segfaults-on

But it seems from duscussion of that ticket that the code in xpra is incorrect as NSTextInputContext must be initialized using init method with a parameter.

Change History (12)

comment:1 Changed 4 weeks ago by Antoine Martin

Description: modified (diff)
Owner: changed from Antoine Martin to unkulunkulu

Thanks for the detailed bug report.

The exact same bug had been reported before: #1725, the workaround we had used was to prevent the context from getting garbage collected.

r18104 should fix this properly this time.

You can find beta python3 / gtk3 2.3 builds with this fix in the macos beta area or you can apply the patch by hand to your installation.

Please close if that works for you.

comment:2 Changed 4 weeks ago by unkulunkulu

As far as I understand you either do .new() OR .alloc().init[...]
Havent tested yet, but what do you think?

comment:3 Changed 4 weeks ago by Antoine Martin

As far as I understand you either do .new() OR .alloc().init[...]

I'm not sure, pyobjc is still a bit of a mistery to me.

comment:4 Changed 4 weeks ago by unkulunkulu

I haven't used it, I judge purely by this intro: https://pythonhosted.org/pyobjc/core/intro.html It says that some classes provide .new as a shortcut to alloc().init

The fix seems to be working, thanks. But from documentation lawyer perspective .alloc() is correct, new() is something like alloc() + init() I guess, for example in this snippet

from AppKit import NSTextInputContext, NSTextView       #@UnresolvedImport
text_input_context = NSTextInputContext.alloc()
view = NSTextView.new()
text_input_context.initWithClient_(view)

del text_input_context

You can change

view = NSTextView.new()

to

view = NSTextView.alloc().init()

but changing it purely to

view = NSTextView.alloc()

also leads to segfault.

comment:5 Changed 4 weeks ago by Antoine Martin

Resolution: fixed
Status: newclosed

I would have thought that initWithClient_ could replace the plain init call, oh well.

If it no longer segfaults, I think I prefer to leave it as it is.

comment:6 Changed 4 weeks ago by unkulunkulu

That's an assumption you for some reason willing to make, ok. Thanks for the fix.

comment:7 Changed 4 weeks ago by Antoine Martin

That's an assumption you for some reason willing to make, ok.

I'm not following you.
What assumption am I making here?
You've proved that we have to call either new() or alloc().init() (since they're both equivalent) and that's exactly what we do. Where is the assumption?

comment:8 Changed 4 weeks ago by unkulunkulu

I see now that you have a call new() and then initWithClient() making an assumption that two calls to init (say the first implicit from new and then explicit initWithClient) are ok.
I thought that changing this line https://xpra.org/trac/browser/xpra/trunk/src/xpra/platform/darwin/keyboard.py#L101 to call to alloc() is strictly according to documentation, without additional assumptions.

comment:9 Changed 4 weeks ago by Antoine Martin

.. to call to alloc() is strictly according to documentation ...

Yes, it would be more correct. That's what I meant in comment:5.

Since I don't have a 10.13.x system to test on, I would have to rely on you to verify that this works as expected. If you can confirm that this works without crashing, I can make the change.

Last edited 4 weeks ago by Antoine Martin (previous) (diff)

comment:10 Changed 4 weeks ago by unkulunkulu

It works, I think you should replace new with alloc

comment:11 Changed 4 weeks ago by Antoine Martin

Done in r18140.

Please let me know if that works, updated beta 2.3 macos python3 build posted here: https://xpra.org/beta/osx/

comment:12 Changed 4 weeks ago by unkulunkulu

The latest version doesn't start like this: #1753
The patch from this ticket however works perfectly (applied it to latest stable from homebrew r17715)
Thank you for lightning fast reaction and help!

Last edited 4 weeks ago by unkulunkulu (previous) (diff)
Note: See TracTickets for help on using tickets.