Spiro segmentation fault #3437
Comments
|
I think I figured out why this happens, |
|
Just figured out how to get |
|
I realize this is not my blog, but I tried to remove Will revert and try other things, but I should admit clearly that I am far from experienced with gdraw and have no idea why this crash occurs. |
|
Paging @jtanx , who perhaps can lend a hand |
|
One wrinkle here is whether closing the dialog using the top controls should be equivalent to "OK" or "Cancel". It might be preferable to request that the window be displayed without those controls, as long as there is still some evident way to drag it. |
|
@skef I think it should be equal to OK ( |
|
I also have another change that unfortunately doesn't work. I hope that posting what doesn't work will help us find what does work. Doesn't work, results in same (or similar) crash as #3437 (comment) |
|
This one had such promise, but results in same crash as #3437 (comment) Running out of ideas. diff --git a/fontforgeexe/cvgetinfo.c b/fontforgeexe/cvgetinfo.c
index 200be05af..46d5299cf 100644
--- a/fontforgeexe/cvgetinfo.c
+++ b/fontforgeexe/cvgetinfo.c
@@ -3752,6 +3752,7 @@ static void SpiroPointGetInfo(CharView *cv, spiro_cp *scp, SplinePointList *spl)
GDrawSetVisible(gip->gw,true);
while ( !gip->done )
GDrawProcessOneEvent(NULL);
+ GDrawDestroyWindow(gip->gw);
}
void CVGetInfo(CharView *cv) {
|
|
Generally clicking 'x' is akin to cancelling the dialogue. I can repro. |
|
I would say that "X" should be interpreted as "OK" in this case, as someone who both uses Spiro and designs fonts in FontForge. |
|
The relevant contrast is how PI_DoCancel is being called. Here is the trace in the Spiro Get Info dialog when you press the actual cancel button: And here it is when you press the "close" decoration:
So the nature of the problem is fairly clear: the function that does the cleanup of the window data ( |
|
Are both of those crashes, or breakpoints? Because pressing the actual cancel button causes no crash for me. |
|
When you click cancel, it goes through here PI_Cancel: https://github.com/fontforge/fontforge/blob/master/fontforgeexe/cvgetinfo.c#L1880-L1881 When you click the close button, it routes through the Note it doesn't call Not entirely sure how the |
|
PS. If what I'm doing is not helping feel free to tell me to stop. |
|
Ok, I think I see, basically unconditionally calling
|
|
@jtanx I tried unconditionally calling it and still get a crash, see above :) |
|
@ctrlcctrlv you need to leave in the PI_DoCancel beforehand. What I said actually doesn't fully make sense, |
breakpoints |
|
@jtanx I don't get what you mean. Removing the condition still results in a crash—I tried both permutations, PI_Destroy first then PI_DoCancel, or PI_DoCancel then PI_Destroy, both result in identical crash. diff --git a/fontforgeexe/cvgetinfo.c b/fontforgeexe/cvgetinfo.c
index 200be05af..809b3ec3f 100644
--- a/fontforgeexe/cvgetinfo.c
+++ b/fontforgeexe/cvgetinfo.c
@@ -1756,11 +1756,8 @@ static void PI_DoCancel(GIData *ci) {
static int pi_e_h(GWindow gw, GEvent *event) {
if ( event->type==et_close ) {
GIData *d = GDrawGetUserData(gw);
- if( d->nonmodal ) {
- PI_Destroy((struct dlistnode *)d);
- } else {
- PI_DoCancel( GDrawGetUserData(gw));
- }
+ PI_DoCancel( GDrawGetUserData(gw));
+ PI_Destroy((struct dlistnode *)d);
} else if ( event->type==et_char ) {
if ( event->u.chr.keysym == GK_F1 || event->u.chr.keysym == GK_Help ) {
help("getinfo.html"); |
|
In any event, the X should result in an OK and not a Cancel! 🙄 |
|
Actually, I think the word "Cancel" should be replaced with "Revert", because as an actual user it's seldom used. This crash has bitten me many times and I've lost beautiful glyphs because of it. Making it undo my work on "X" would be the same as a crash, bad. |
|
I'm going to leave this for tonight, but just some notes: I had if ( event->type==et_close ) {
GIData *d = GDrawGetUserData(gw);
if( !d->nonmodal ) {
PI_DoCancel(d);
}
PI_Destroy((struct dlistnode *)d);Which wasn't crashing for me. I think the reason why it's conditional is because of the behaviour you mention - for normal points, it doesn't revert when you click the close button, So to have that behaviour with Spiro points, you just don't call Not entirely sure what that means for cleaning up resources though. |
|
Here's what you wrote as a patch: diff --git a/fontforgeexe/cvgetinfo.c b/fontforgeexe/cvgetinfo.c
index 200be05af..ae3949729 100644
--- a/fontforgeexe/cvgetinfo.c
+++ b/fontforgeexe/cvgetinfo.c
@@ -1757,10 +1757,9 @@ static int pi_e_h(GWindow gw, GEvent *event) {
if ( event->type==et_close ) {
GIData *d = GDrawGetUserData(gw);
if( d->nonmodal ) {
- PI_Destroy((struct dlistnode *)d);
- } else {
PI_DoCancel( GDrawGetUserData(gw));
- }
+ }
+ PI_Destroy((struct dlistnode *)d);
} else if ( event->type==et_char ) {
if ( event->u.chr.keysym == GK_F1 || event->u.chr.keysym == GK_Help ) {
help("getinfo.html");It crashes for me: |
|
Can you try it on a fresh font file? That looks like a potentially unrelated crash. |
|
I click "New" each time. |
|
Try running fontforge with a breakpoint on |
|
@ctrlcctrlv : Your patch left out the |
|
Still crashes even with the forgotten exclam...sorry about that by the way, that's why I always communicate in patches, not snippets. :-) diff --git a/fontforgeexe/cvgetinfo.c b/fontforgeexe/cvgetinfo.c
index 200be05af..3433e4463 100644
--- a/fontforgeexe/cvgetinfo.c
+++ b/fontforgeexe/cvgetinfo.c
@@ -1756,11 +1756,10 @@ static void PI_DoCancel(GIData *ci) {
static int pi_e_h(GWindow gw, GEvent *event) {
if ( event->type==et_close ) {
GIData *d = GDrawGetUserData(gw);
- if( d->nonmodal ) {
- PI_Destroy((struct dlistnode *)d);
- } else {
- PI_DoCancel( GDrawGetUserData(gw));
- }
+ if( !d->nonmodal ) {
+ PI_DoCancel(d);
+ }
+ PI_Destroy((struct dlistnode *)d);
} else if ( event->type==et_char ) {
if ( event->u.chr.keysym == GK_F1 || event->u.chr.keysym == GK_Help ) {
help("getinfo.html"); |
|
If you save as an sfd before opening the dialog, and then open fontforge with that file and immediately open the dialog on the middle point, does it reproduce? If so, can you attach the file you created in the earlier step? |
|
KDE/Kwin |
|
Plasma? |
|
Yes |
|
Huh, so our systems are unusually close, then. Would it be possible to determine if |
|
I set a breakpoint just now but it never triggers. The first time I close the dialog, nothing appears to happen. The second time, as above, the crash occurs in |
|
OK, so you're still seeing the dialog not close on the first try. Hmm. There are straightforward ways of tracking this down but it will take a little bit of research. Let's leave it there for now, I'll try to update later with a "strategy" and things can go from there. |
|
As @jtanx asked, I just did this: The first time, I press X, nothing seems to happen, the UI changes not at all, only breakpoint triggers. The second time, crash! |
|
Oh, no, wait this is very simple. In your patch you've replaced Added: No, I'm wrong, those should be the same. |
|
@skef Uh, so did @jtanx - #3437 (comment) I really would appreciate if in the future we all communicated in standard patch format rather than fallible humans copying and pasting code (or rewriting it) 😉 |
|
Could this possibly have something to do with it? Is my invocation of gdb wrong? |
|
Perhaps it's loading the libraries from the system FontForge while running the local binary...just a wild guess. |
|
Even if I do |
|
Those warnings are pretty common. But it couldn't hurt to "make clean" and do a fresh build. If you need to confirm that the tests are running the compiled code you can always add something that just won't work. However, you've already seen different symptoms from different code combinations so that's less likely. |
|
OK, yeah if that's all I need to do to confirm then yes I'm using the right code because I put "Q" in a few strings in the dialog so I'd know which I'm using. (I stash those silly changes before diffing, hehe.) |
|
In that case, what's the complete diff from the current master that you're testing with? |
|
If this is enough to cause problems, we all should give up because FontForge is far too brittle for sane men to ever work on :o) diff --git a/fontforgeexe/cvgetinfo.c b/fontforgeexe/cvgetinfo.c
index 200be05af..5c9c9b6d0 100644
--- a/fontforgeexe/cvgetinfo.c
+++ b/fontforgeexe/cvgetinfo.c
@@ -1756,11 +1756,10 @@ static void PI_DoCancel(GIData *ci) {
static int pi_e_h(GWindow gw, GEvent *event) {
if ( event->type==et_close ) {
GIData *d = GDrawGetUserData(gw);
- if( d->nonmodal ) {
- PI_Destroy((struct dlistnode *)d);
- } else {
- PI_DoCancel( GDrawGetUserData(gw));
- }
+ if( !d->nonmodal ) {
+ PI_DoCancel(d);
+ }
+ PI_Destroy((struct dlistnode *)d);
} else if ( event->type==et_char ) {
if ( event->u.chr.keysym == GK_F1 || event->u.chr.keysym == GK_Help ) {
help("getinfo.html");
@@ -3240,7 +3239,7 @@ static void PointGetInfo(CharView *cv, SplinePoint *sp, SplinePointList *spl) {
/* Why 3? */
mgcd[j].gd.pos.x = mgcd[j-2].gd.pos.x-50+3; mgcd[j].gd.pos.y = mgcd[j-1].gd.pos.y+26;
mgcd[j].gd.flags = gg_visible | gg_enabled;
- mlabel[j].text = (unichar_t *) _("Prev On Contour");
+ mlabel[j].text = (unichar_t *) _("QPrev On Contour");
mlabel[j].text_is_1byte = true;
mgcd[j].gd.label = &mlabel[j];
mgcd[j].gd.cid = CID_PrevC;
@@ -3250,7 +3249,7 @@ static void PointGetInfo(CharView *cv, SplinePoint *sp, SplinePointList *spl) {
mgcd[j].gd.pos.x = mgcd[j-2].gd.pos.x; mgcd[j].gd.pos.y = mgcd[j-1].gd.pos.y;
mgcd[j].gd.flags = gg_visible | gg_enabled;
- mlabel[j].text = (unichar_t *) _("Next On Contour");
+ mlabel[j].text = (unichar_t *) _("QNext On Contour");
mlabel[j].text_is_1byte = true;
mgcd[j].gd.label = &mlabel[j];
mgcd[j].gd.cid = CID_NextC;
|
|
Sometimes in these cases of weird differences it helps to follow up the possibilities without judgment. But I've just tried this exact code and I don't see the problem. The next step is to find out specifically what's happening the first time you hit the close button. But I don't have a "recipe" for that at the moment. |
|
I just determined there's a difference between running |
Good news!Right before dozing off I thought of some keywords I just had to try to search for. I found a solution to my problem, and that problem was causing the crash. I confirm this patch works: #3437 (comment) I believe this command, the proper invocation of Thank you so much to everyone who helped. When I wake up tomorrow I'll make documentation PR's. |
|
I lazily avoid these problems by just Regardless, good to hear we have this pinned down. |
Clarify the correct way to debug FontForge to prevent a recurrence of fontforge#3437 (comment), a very subtle bug.
|
Right, yeah makes sense in retrospect, your backtraces were referencing the line numbers of the unmodified code. I also just go with This did however, highlight a bug in gdk - the way I got it to crash was to open the point info window, then go back to the font view, attempt to activate another charview, then try to close the point info. This screws up the undo state that the point info dialogue expects, but gxdraw disallows this. |
|
Fixed in ctrlcctrlv@42c95d4 |
Clarify the correct way to debug FontForge to prevent a recurrence of fontforge#3437 (comment), a very subtle bug.
Tested on Arch Linux, commit c96d3eb
The text was updated successfully, but these errors were encountered: