xpra icon
Bug tracker and wiki

Opened 10 months ago

Closed 10 months ago

Last modified 10 months 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 10 months 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 10 months 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 10 months 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 10 months 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 10 months 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 10 months ago by unkulunkulu

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

comment:7 Changed 10 months 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 10 months 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 10 months 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 10 months ago by Antoine Martin (previous) (diff)

comment:10 Changed 10 months ago by unkulunkulu

It works, I think you should replace new with alloc

comment:11 Changed 10 months 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 10 months 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 10 months ago by unkulunkulu (previous) (diff)
Note: See TracTickets for help on using tickets.