Check y for NULL too, remove duplicate zero'ing#919
Check y for NULL too, remove duplicate zero'ing#919RobLoach merged 2 commits intoImmediate-Mode-UI:masterfrom
Conversation
| struct nk_page_element *elem; | ||
| elem = nk_create_page_element(ctx); | ||
| if (!elem) return 0; | ||
| nk_zero_struct(*elem); |
There was a problem hiding this comment.
(haven't read nor tested anything else yet)
I noticed a redundant nk_zero() call, so I removed it.
There is one other function that does the very same thing - nk_create_panel - both of these don't do anything meaningful other than zero'ing memory, so it's a bit suspicious to remove the only meaningful line from it, however, you're right nk_create_page_element already zero'es the values.
I would suggest to remove the same line from nk_create_panel in order to keep it consistent, but honesty I feel uneasy any of this is being done here, since it isn't really affecting your fix in any way.
There was a problem hiding this comment.
(haven't read nor tested anything else yet)
I noticed a redundant nk_zero() call, so I removed it.
There is one other function that does the very same thing - nk_create_panel - both of these don't do anything meaningful other than zero'ing memory, so it's a bit suspicious to remove the only meaningful line from it, however, you're right nk_create_page_element already zero'es the values.
I would suggest to remove the same line from nk_create_panel in order to keep it consistent, but honesty I feel uneasy any of this is being done here, since it isn't really affecting your fix in any way.
Both nk_create_table() and nk_craete_panel() do something besides the nk_zero() it's just very minimal. They return the pointer of the correct type of the union so the caller doesn't have to.
But I agree, I should remove it from both, and yeah it should probably be its own commit, it just bothers me. There's so many little things (even typos) that I want to just slip in sometimes because it doesn't seem worth it to create a whole other branch + pull request to wait on.
RobLoach
left a comment
RobLoach
left a comment
There was a problem hiding this comment.
Seems fine. I agree that other suggestion is a good one, but may live in a different PR better.
K, I'll look into avoiding unnecessary table use when I get the chance, and create another PR for removing the redundant nk_zero calls (maybe I will throw some other similar minor fixes into that one though). |
|
Splitting to another commit should be fine. Just make sure to rework commit history during rebase, so there will be only two when this gets merged (take a look at kimgr/git-rewrite-guide in case of troubles) With the changes done to both functions, this should be okey. I don't mind it being in the same PR. |
Are you talking to me or Rob here? I can try rewriting the history into 2 commits and doing a force push... |
|
Can Squash when bringing in, but there's only two commits. While resolving git conflicts is annoying, this only has ~50 line changes so it should be easy if there are big problems. |
I think I might try fixing it myself anyway. I don't think I've ever done an interactive rebase before and something with 2 commits and such simple changes seems like a good time to practice. Just having to pull out one line change from the first commit and add it and another to the second.... what could go wrong? ha |
To you :) Seems like Rob didn't read our conversation fully. I was also confused about his request.
Don't worry. Even if you force-push something by mistake, Github tracks all those changes. Nothing ever gets lost in PR branches. |
There are some rare cases where the x and y offsets are split across tables and the table with y gets incorrectly garbage collected in nk_clear(). This prevents that from causing a segfault.
|
Well I rebased and logically split the commits. As far as I know there is no way to avoid a force push when you do a rebase since the tags change even if nothing else. EDIT: English, do I speak it?? |
There was a problem hiding this comment.
Thanks.
I've looked into the diffs, and this feels more like a workaround rather than a fix, since y_offset disappearing should (probably) never happen in the first place. Did you notice the window to rapidly "flicker" when any of those new branches are being hit? I imagine this would be a symptom.
Anyway, that code was already doing many questionable things, and I don't have any "real" proof against this patch, so I'm going to trust you and Rob's judgement.
No flicker. Like I said there is a flaw in the way the "garbage collection" determines and I don't think there is a fix without larger more serious changes which probably aren't worth it, at least not right this second as they would require quite a bit of thought (and likely debate). For instance, the fact that iirc, both groups and windows are treated the same for the purposes of seq and subsequent garbage collection when they shouldn't be, and how even aside from that, a single integer seq is probably insufficient information to determine if a table can be freed. We could just remove garbage collection for tables. Or we could have a "touched" flag for each table that is reset each frame which makes determining whether it's garbage straightforward, but I would need to look into whether table.seq is used for anything else and can be removed and whether there are any other places where it is misused, then if those misuses require different fixes or not, because if there's flawed logic here, it's entirely likely there is similar elsewhere.
Up to you guys if you want me to try for a deeper fix now or just get this bug fix in and look into that later. I would probably try go for option B above. |
|
Let's merge it and hope for the best, at least for now. Even if it's wrong, I would only imagine this thing resetting the scrollbar/content position in some very rare cases. Probably way better than getting a random crash. The garbage collection logic is already giving me nightmares (#896 (comment)) and I would sleep more peacefully knowing no one is trying to actively change it. @RobLoach remember to NOT squash it :) |
Oh I have no worries that it's wrong. Logically it can't be. The new code only applies in the edge case that causes a crash when they both would have been set to 0 on creation anyway if they were in the same garbage collected table. My only concern was as you said it is a fix of an underlying problem and adding 1 additional branch over resolving the real issue. |
3 participants
Ran into this bug today. I don't have a simple standalone program to reproduce it, but my current project was triggering it consistently.
What happens is that the x and y offsets are split across two different tables and then the table with the y offset gets removed in nk_clear() because table->seq != ctx->seq at some point. Then the next frame, x is found so it assumes y exists and viola, you have a segfault.
Clearly the underlying logic of how it uses seq to determine what is safe to free is flawed but I don't know what the best way to fix it.
I think this is a very rare case that is happening because I have a large number of groups (a single table holds 69 items or 34.5 x/y pairs) in a "tabbed" window so what happens as I click through the tabs more groups get added to the table and eventually we get the x/y table split. Then when you switch to another tab you "touch" at least some of the items in the first table thus updating its seq member to the current ctx->seq, but not the newest table with the y member of the no-longer visible group, so that table gets tossed. Finally, switching back to the that tab it still finds the x offset in the first table and assumes the y must also exist even though it doesn't.
One thing I didn't look deeply into but I should is if we can avoid this code entirely when NK_WINDOW_NO_SCROLLBAR is set. It's just a waste of time/space and most groups are used for simple layouting purposes and so will not need a scrollbar. Complex arrangements require lots of groups, thus making running into this issue more likely. Just a thought.
In the process I noticed a redundant nk_zero() call, so I removed it.
@RobLoach @sleeptightAnsiC