Crash in paintImage

[pbrown12303]

Checklist

• I have searched the issue tracker for open issues that relate to the same problem, before opening a new one.
• This issue only relates to a single bug. I will open new issues for any other problems.

Describe the bug

Had a crash today in paintImage. The stack shows the Resource as nil and the code checks for being not nil before the failed line, I have seen similar errors lately in which apparently checking an interface for nil is not the same as checking the value of the interface for nil (https://stackoverflow.com/questions/13476349/check-for-nil-and-nil-interface-in-go). In this case the type is present, but the concrete value is nil. A simple check for nil returns false because the type is present.

I believe the error is in fyne/v2/internal/painter/image.go

Line 87 currently reads:

	case img.File != "" || img.Resource != nil:

I believe it should read:

	case img.File != "" || (img.Resource != nil && !reflect.ValueOf(img.Resource).IsNil()):

How to reproduce

Run sample code below

Screenshots

Example code

	testApp := app.New()
	w := testApp.NewWindow("Test Nil Resource Rendering")
	w.Resize(fyne.NewSize(600, 600))

	var staticRes *fyne.StaticResource = nil
	var res fyne.Resource = staticRes

	img := canvas.NewImageFromResource(res)
	img.SetMinSize(fyne.NewSize(30, 30))
	img.Resize(fyne.NewSize(30, 30))
	diagramContainer := container.NewWithoutLayout(img)
	img.Move(fyne.NewPos(100, 100))

	w.SetContent(diagramContainer)
	w.ShowAndRun()

Fyne version

2.3.5

Go compiler version

1.20.7

Operating system and version

Windows 10 22H2

Additional Information

No response

[pbrown12303]

Here's the crash report
CrashReport.txt

[Jacalz]

As per the issue template, please include example code that we can use to replicate this. The example code section is required for a reason and should not be ignored.

[pbrown12303]

I have updated with the requested information and a proposed fix.

[andydotxyz]

Interesting fix. I'm not certain if libraries are expected to catch this case. When using others (some builtin) I'm sure that such "non-nil" nils had to be fixed in my code.
Is the root cause in Fyne or is the resource with nil content coming from your app?

[Jacalz]

I agree that a nil resource kind of feels like a bug in the application and not necessarily in fyne. Doing a slow reflect call each time we paint also doesn’t seem very desirable.

[andydotxyz]

Doing a slow reflect call each time we paint also doesn’t seem very desirable.

Yeah that was got me thinking as well.

[pbrown12303]

First of all, I doubt the reflect.ValueOf() call carries any significant overhead - it's just the retrieval of a struct field from a standard two-field struct, i.e. the interface type is just a two-field struct. There's no finding and parsing of a struct definition or anything like that.

Secondly, the fact is that the rendering occurs in a separate thread from the working thread that modifies the data structures. This makes it very very difficult to determine where in the user code the null value was created. You can't just place a breakpoint and look back up the stack to figure out where it came from.

Given these two considerations, my recommendation is to apply the suggested fix. There is virtually no overhead, and it makes the rendering robust with respect to a perfectly valid (though unexpected) data structure. Defensive programming.

[pbrown12303]

Here's the source to IsNil(). Virtually no overhead.

// IsNil reports whether its argument v is nil. The argument must be
// a chan, func, interface, map, pointer, or slice value; if it is
// not, IsNil panics. Note that IsNil is not always equivalent to a
// regular comparison with nil in Go. For example, if v was created
// by calling ValueOf with an uninitialized interface variable i,
// i==nil will be true but v.IsNil will panic as v will be the zero
// Value.
func (v Value) IsNil() bool {
	k := v.kind()
	switch k {
	case Chan, Func, Map, Pointer, UnsafePointer:
		if v.flag&flagMethod != 0 {
			return false
		}
		ptr := v.ptr
		if v.flag&flagIndir != 0 {
			ptr = *(*unsafe.Pointer)(ptr)
		}
		return ptr == nil
	case Interface, Slice:
		// Both interface and slice are nil if first word is 0.
		// Both are always bigger than a word; assume flagIndir.
		return *(*unsafe.Pointer)(v.ptr) == nil
	}
	panic(&ValueError{"reflect.Value.IsNil", v.kind()})
}

[pbrown12303]

Here's the source to ValueOf():

// ValueOf returns a new Value initialized to the concrete value
// stored in the interface i. ValueOf(nil) returns the zero Value.
func ValueOf(i any) Value {
	if i == nil {
		return Value{}
	}

	if !go121noForceValueEscape {
		escapes(i)
	}

	return unpackEface(i)
}

[pbrown12303]

And the source to unpackEface():

// unpackEface converts the empty interface i to a Value.
func unpackEface(i any) Value {
	e := (*emptyInterface)(unsafe.Pointer(&i))
	// NOTE: don't read e.word until we know whether it is really a pointer or not.
	t := e.typ
	if t == nil {
		return Value{}
	}
	f := flag(t.Kind())
	if t.IfaceIndir() {
		f |= flagIndir
	}
	return Value{t, e.word, f}
}

[Jacalz]

I'm always hesitant when it comes to adding reflect usages. There is almost as always a better way than using reflection. It is also worth noting that, as far as I know, importing reflect makes the compiler sometimes include more information about types and thus making binaries larger.

I understand that the bug on you side is hard to track down but making the painter just ignore it doesn't seem to be helpful at all. It seems better to me that it crashes and you actually know that there is a bug in your code than sweeping it under the rug.

[pbrown12303]

Yes, I see that. But my point is really that the data structure is actually valid. Technically, it is not in error. Any code that has a *StaticResource variable can produce this problem.

…

-- PCB

_______________________________________________________________ Paul C. Brown Paul C. Brown Consulting LLC - *Specializing in IT Architecture and Strategy* "Total architecture is not a choice - it is a concession to reality." Email: pbrow ***@***.***>***@***.*** Mobile: 518-424-5360 Architecture Books:

-- Succeeding With SOA: Realizing Business Value Through Total Architecture <http://www.informit.com/authors/bio.aspx?a=53cdb713-432d-4e63-a5b1-3a9ee6fedbeb>

-- Implementing SOA: Total Architecture In Practice <http://www.informit.com/authors/bio.aspx?a=53cdb713-432d-4e63-a5b1-3a9ee6fedbeb>

-- TIBCO Architecture Fundamentals <http://www.informit.com/authors/bio.aspx?a=53cdb713-432d-4e63-a5b1-3a9ee6fedbeb>

-- Architecting Composite Applications and Services with TIBCO <http://www.informit.com/authors/bio.aspx?a=53cdb713-432d-4e63-a5b1-3a9ee6fedbeb>

-- Architecting Complex-Event Processing Solutions with TIBCO <http://www.informit.com/authors/bio.aspx?a=53cdb713-432d-4e63-a5b1-3a9ee6fedbeb> The SOA Manifesto: soa-manifesto.or <http://soa-manifesto.org/>g

______________________________________________________________

On Fri, Aug 11, 2023 at 12:09 PM Jacob Alzén ***@***.***> wrote: I'm always hesitant when it comes to adding reflect usages. There is almost as always a better way than using reflection. It is also worth noting that, as far as I know, importing reflect makes the compiler sometimes include more information about types and thus making binaries larger. I understand that the bug on you side is hard to track down but making the painter just ignore it doesn't seem to be helpful at all. It seems better to me that it crashes and you actually know that there is a bug in your code than sweeping it under the rug. — Reply to this email directly, view it on GitHub <#4144 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEODRDHVWCVPURTFQLTIJ2DXUZKNHANCNFSM6AAAAAA3IUS6DQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[Jacalz]

I have updated with the requested information and a proposed fix.

I just want to say for the record that please add a fully runnable example next time and not just part of the main function. You know that until the next time now but you don't need to update anything now.

[andydotxyz]

I guess what we need to do is understand best practice for libraries in this space, or to make a policy ourselves.
I know that this will not be a one-off - we check for nil in many places that will fail if we get a type-with-nil instead...

[Jacalz]

That type with nil issue seems a like a severe problem with how Go implements interfaces (probably not a bug in the usual sense; it might just be a downside of their approach to the interface problem). Not necessarily a bug in our code. If we were to check for that everywhere we'd be doing reflect calls all over the place and that doesn't seem sensible to me.

It is a bit like with nil canvas objects and widgets. We don't really check for it (layouts etc.) because it shouldn't happen. If the developer creates nil objects, it is a bug in their code if anything.