Replies: 32 comments 15 replies
-
|
A couple of my PRs are related to fixing this issue, so be sure to refactor this code, it will be a one-and-done exercise |
Beta Was this translation helpful? Give feedback.
-
|
Those assertions should never fail. Can you file an issue with the specific stack trace of one of the crashes along with instructions on how to reproduce it? |
Beta Was this translation helpful? Give feedback.
-
I'm going to take the time to make a demo code that reproduces the bug, but I don't know if it will be as expected, it's a bit like Vulkan's out-of-memory bug, and the previous fix is still problematic |
Beta Was this translation helpful? Give feedback.
-
package main
//go:generate core generate
import (
"cogentcore.org/core/gi"
"cogentcore.org/core/giv"
"cogentcore.org/core/icons"
)
type TableStruct struct { //gti:add
Icon icons.Icon
IntField int `default:"2"`
FloatField float32
StrField string
File gi.Filename
}
func main() {
tsttable := make([]*TableStruct, 0)
b := gi.NewBody("leak")
tv := giv.NewTableView(b, "tv")
tv.SetReadOnly(true)
tv.SetSlice(&tsttable)
go func() {
for i := 0; i < 100000; i++ {
tsttable = append(tsttable, &TableStruct{IntField: i, FloatField: float32(i) / 10.0})
tv.SetSlice(&tsttable)
if len(tsttable) > 0 {
tv.ScrollToIdx(len(tsttable) - 1)
}
}
}()
b.RunMainWindow()
} |
Beta Was this translation helpful? Give feedback.
-
|
I still recommend using this lib for security detection https://github.com/uber-go/nilaway |
Beta Was this translation helpful? Give feedback.
-
|
Maybe using more inspection code will make the code readable a little uglier, but with the upgrade of the new version of go, a lot of redundant code should be cleaned up, and secondly, a lot of assertions where I wonder if we can use generics to constrain types, plus null pointer checking, slice out-of-bounds checking, fuzz testing, stress testing, unit testing, code coverage testing, and lock analysis for go trance, etc., Let core write layout code at will at will for any user, no longer panic and run smoothly. In this state, it will be very stable for us to expand new widgets. As in the demo code above, a lot of type assertion failures are triggered, and null pointers panic... Too many crashes, I hope you will consider my suggestion again, thanks. |
Beta Was this translation helpful? Give feedback.
-
|
There are thousands of suggestions by nilaway, and almost all of them will never trigger panics. Instead of overprotecting the code, which will make it much uglier as you said, we should just fix the rare cases in which more safety is needed, like your window closing crash (#795), which I am working on fixing right now. |
Beta Was this translation helpful? Give feedback.
-
|
Then please test the above demo code, try to drag to change the window size, whether it will panic
…---Original---
From: ***@***.***>
Date: Sun, Feb 11, 2024 01:18 AM
To: ***@***.***>;
Cc: ***@***.******@***.***>;
Subject: Re: [cogentcore/core] refactor for slice and assert interfere(Discussion #858)
There are thousands of suggestions by nilaway, and almost all of them will never trigger panics. Instead of overprotecting the code, which will make it much uglier as you said, we should just fix the rare cases in which more safety is needed, like your window closing crash (#795), which I am working on fixing right now.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Beta Was this translation helpful? Give feedback.
-
|
I am aware that it panics and will work on fixing that panic. |
Beta Was this translation helpful? Give feedback.
-
|
In addition to the demo above, the panic in the rest of the situation is difficult to reproduce on different machines, I am curious why you don't debug the bug directly on my machine, this will save you the time of mocking my machine environment configuration, such as vulkan bug, I vaguely feel that it is not easy to reproduce on other machines, I found that the stable reproduction method: minimized. I've been testing for nearly half a month before I found that the minimization can be reproduced every time, and the first 14 days can only sit and wait for it to crash, if you want to write a demo like the above can reproduce the bug on any machine, I don't think it's realistic.
…---Original---
From: ***@***.***>
Date: Sun, Feb 11, 2024 01:18 AM
To: ***@***.***>;
Cc: ***@***.******@***.***>;
Subject: Re: [cogentcore/core] refactor for slice and assert interfere(Discussion #858)
There are thousands of suggestions by nilaway, and almost all of them will never trigger panics. Instead of overprotecting the code, which will make it much uglier as you said, we should just fix the rare cases in which more safety is needed, like your window closing crash (#795), which I am working on fixing right now.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Beta Was this translation helpful? Give feedback.
-
|
If I can not easily reproduce it on the other machine I am going to test, I will consider debugging it on your machine. |
Beta Was this translation helpful? Give feedback.
-
|
On my other Windows device, I can reproduce the window closing issue, but I can not reproduce the Vulkan memory issue. Can you clarify whether the Vulkan bug has only been happening for 15 days, or has it been happening before then? Is it possible that I made a change around 15 days ago that introduced the bug? |
Beta Was this translation helpful? Give feedback.
-
|
Can you run |
Beta Was this translation helpful? Give feedback.
-
|
I'm sorry I'm asleep, it's 6:20 a.m. in the middle of the night, maybe our time zones are a couple of hours apart. According to my guess, it should have existed for half a month, because half a month ago I was still studying the layout in the example folder and the construction of repositories such as gide, and I was not familiar with how to operate code such as widgets and layouts, and in the past half a month, I transplanted the program interface developed by several other UI libraries to the core, and I found this bug after constantly accessing the back-end business code of various practical scenarios
…---Original---
From: ***@***.***>
Date: Sun, Feb 11, 2024 02:07 AM
To: ***@***.***>;
Cc: ***@***.******@***.***>;
Subject: Re: [cogentcore/core] refactor for slice and assert interfere(Discussion #858)
On my other Windows device, I can reproduce the window closing issue, but I can not reproduce the Vulkan memory issue. Can you clarify whether the Vulkan bug has only been happening for 15 days, or has it been happening before then? Is it possible that I made a change around 15 days ago that introduced the bug?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Beta Was this translation helpful? Give feedback.
-
|
No worries, it is fine that our time zones are different. Can you run |
Beta Was this translation helpful? Give feedback.
-
|
Beta Was this translation helpful? Give feedback.
-
|
Come on, get on anydesk
…---Original---
From: ***@***.***>
Date: Sun, Feb 11, 2024 06:32 AM
To: ***@***.***>;
Cc: ***@***.******@***.***>;
Subject: Re: [cogentcore/core] refactor for slice and assert interfere(Discussion #858)
No worries, it is fine that our time zones are different. Can you run git checkout 6c39800 in core and then see if the Vulkan issue still happens when you run the demo? That version is from a month ago, so that will test whether this is a new issue or not.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Beta Was this translation helpful? Give feedback.
-
|
I will try it now. |
Beta Was this translation helpful? Give feedback.
-
|
I got disconnected from the anydesk, but I think I have figured out the solution to the window size issue now. I will implement the vulkan crash and window size solutions, and then you can pull and tell me whether they work. You can reset all of the changes I made on your machine. |
Beta Was this translation helpful? Give feedback.
-
|
Hi, please go back to my machine again and watch it for 1 minute,Zoom 100% is too big, can you default to 80% or 70%.
…---Original---
From: ***@***.***>
Date: Sun, Feb 11, 2024 08:41 AM
To: ***@***.***>;
Cc: ***@***.******@***.***>;
Subject: Re: [cogentcore/core] refactor for slice and assert interfere(Discussion #858)
I got disconnected from the anydesk, but I think I have figured out the solution to the window size issue now. I will implement the vulkan crash and window size solutions, and then you can pull and tell me whether they work. You can reset all of the changes I made on your machine.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Beta Was this translation helpful? Give feedback.
-
Details
GOROOT=C:\Users\Admin\go\pkg\mod\golang.org\[email protected] #gosetup 2024/02/15 06:31:50 ----- END OF STACK TRACE ----- goroutine 23 [running]: 进程 已完成，退出代码为 2 |
Beta Was this translation helpful? Give feedback.
-
|
nice
…---Original---
From: ***@***.***>
Date: Wed, Feb 28, 2024 23:56 PM
To: ***@***.***>;
Cc: ***@***.***>;"State ***@***.***>;
Subject: Re: [cogentcore/core] refactor for slice and assert interfere(Discussion #858)
Yes, we should be able to do all of those things in unit tests, and I am planning to start writing more unit tests today.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you modified the open/close state.Message ID: ***@***.***>
|
Beta Was this translation helpful? Give feedback.
-
|
This is also true for slice overflow checking, as long as you add an elemByIndex method to the interface signature, all interfaces will mock an instance of nil in the package where the interface is located |
Beta Was this translation helpful? Give feedback.
-
|
For null pointer panic, I found that 98% of the panic was due to interface assertions |
Beta Was this translation helpful? Give feedback.
-
|
If that works, we need to define a common IsValider interface and then have all interfaces nest it anonymously, so that nothing slips through the net in all polymorphic instances. This is the perfect solution I can think of at the moment, even if you don't approve of it, I want to try to make a wave of big changes, maybe you didn't notice that I made a PR today, and about 9 commits are to solve this kind of problem, and the previous few PRs are also this kind of problem, and it will not be cured for a long time. How important it is for me to have a secure assertion and a concurrency-safe map. |
Beta Was this translation helpful? Give feedback.
-
|
I strongly believe in the value of safe code, and it is one of my highest priorities to fix these crashes for good. However, your solution of a mocked nil implementation of the interface is an excessively complicated solution that I do not think makes sense. I will continue to work on figuring out these crashes, and I will try to come up with a better solution. Please do not rewrite all of the code using your solution until we figure out a better solution. |
Beta Was this translation helpful? Give feedback.
-
|
okay
…---Original---
From: ***@***.***>
Date: Thu, Feb 29, 2024 08:08 AM
To: ***@***.***>;
Cc: ***@***.***>;"State ***@***.***>;
Subject: Re: [cogentcore/core] refactor for slice and assert interfere(Discussion #858)
I strongly believe in the value of safe code, and it is one of my highest priorities to fix these crashes for good. However, your solution of a mocked nil implementation of the interface is an excessively complicated solution that I do not think makes sense. I will continue to work on figuring out these crashes, and I will try to come up with a better solution. Please do not rewrite all of the code using your solution until we figure out a better solution.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you modified the open/close state.Message ID: ***@***.***>
|
Beta Was this translation helpful? Give feedback.
-
|
All panic is controllable at the moment, so turn it off to free up time for other transactions |
Beta Was this translation helpful? Give feedback.
-
|
Today I came up with another way to write it // Type Let's make interface assertions as safe as rust first, and only deal with null pointers last,
// since the source of null pointers can be interface assertions
type Type interface {
*SliceViewBase |
*gi.Slider |
*gi.Label |
*gi.Button
}
// AssertInterface all check in one func
func AssertInterface[T Type](point T, callBack func()) {
switch point := any(point).(type) {
case *SliceViewBase:
if point == nil {
grr.Log(errors.New("point == nil"))
return
}
if point.This() == nil {
grr.Log(errors.New("point.This() == nil"))
return
}
if _, ok := point.This().(SliceViewer); !ok {
grr.Log(errors.New("point.This().(SliceViewer) not ok"))
return
}
sliceGrid := point.This().(SliceViewer).SliceGrid()
if sliceGrid != nil {
if len(sliceGrid.Kids) == 0 { //we need check slice
grr.Log(errors.New("len(sliceGrid.Kids) == 0"))
return
}
if _, ok := sliceGrid.Kids[0].(gi.Widget); !ok {
grr.Log(errors.New("sliceGrid.Kids[0].(gi.Widget) not ok"))
return
}
} else {
grr.Log(errors.New("point.This().(SliceViewer).SliceGrid() == nil"))
return
}
callBack() //this wil be 100% safe
case *gi.Slider:
case *gi.Label:
case *gi.Button:
}
}
func (sv *SliceViewBase) UpdateScroll() {
//sg := sv.This().(SliceViewer).SliceGrid()
//if sg == nil {
// return
//}
//sg.UpdateScroll(sv.StartIdx)
AssertInterface(sv, func() {
sv.This().(SliceViewer).SliceGrid().UpdateScroll(sv.StartIdx)
})
}
//func (sv *SliceViewBase) RowFirstWidget(row int) (*gi.WidgetBase, bool) {
// if !sv.Is(SliceViewShowIndex) {
// return nil, false
// }
// if !sv.IsRowInBounds(row) {
// return nil, false
// }
// nWidgPerRow, _ := sv.This().(SliceViewer).RowWidgetNs()
// sg := sv.This().(SliceViewer).SliceGrid()
// w := sg.Kids[row*nWidgPerRow].(gi.Widget).AsWidget()
// return w, true
//}
func (sv *SliceViewBase) RowFirstWidget(row int) (w *gi.WidgetBase, ok bool) {
Assert(sv, func() {
if !sv.Is(SliceViewShowIndex) {
return
}
if !sv.IsRowInBounds(row) {
return
}
nWidgPerRow, _ := sv.This().(SliceViewer).RowWidgetNs()
sg := sv.This().(SliceViewer).SliceGrid()
w = sg.Kids[row*nWidgPerRow].(gi.Widget).AsWidget()
})
return w, true
}
func (sv *SliceViewBase) SliceNewAtRow(row int) {
//sv.This().(SliceViewer).SliceNewAt(sv.StartIdx + row)
AssertInterface(sv, func() {
sv.This().(SliceViewer).SliceNewAt(sv.StartIdx + row)
})
}
|
Beta Was this translation helpful? Give feedback.
-
|
Please check this proposal carefully, I feel it's better than the last one, the readability is rather a bit better, and for me, the centralized detection is not only for interface assertions, but even for slice transgressions |
Beta Was this translation helpful? Give feedback.
-
|
We can even remove a lot of the "if xxoo == nil {" sentences inside the previous code, and these judgments will be unified within a single function |
Beta Was this translation helpful? Give feedback.
-
|
Again, I appreciate your suggestions, but I think that we do not want to use an idiosyncratic API that is orthogonal to the standard Go safety paradigm. Go code is not somehow irrevocably less safe than Rust code; we just have certain bugs that we are working on fixing. I have ideas for fixing all of the currently raised panic issues, and I just haven't had time to implement them yet. I will work on implementing them soon. |
Beta Was this translation helpful? Give feedback.
-
When I use this command
go list ./... | grep -v /vendor/ | xargs -n1 go run
to test all the main functions, I still find that many types of assertions will panic, so I propose to refactor all the code of type assertions and slicing operations to add various checks, in addition, just returning a failure may cause trouble to obtain debugging information, so when it fails, we should output the assertion failure or array out-of-bounds to the log?
Beta Was this translation helpful? Give feedback.
All reactions