win32: Window.buffer manipulation is racy, can panic on minimisation

[sqweek]

I've seen it panic once when minimising a window, like so:

panic: runtime error: index out of range

goroutine 51 [running]:
github.com/AllenDang/w32.SetDIBitsToDevice(0xfffffffff4013245, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x5969d0, ...)
        C:/src/go/src/github.com/AllenDang/w32/gdi32.go:471 +0x10a
github.com/skelterjohn/go.wde/win.(*Window).blitImage(0xc04206a280, 0xfffffffff4013245, 0xc0430b29c0)
        C:/src/go/src/github.com/skelterjohn/go.wde/win/win_windows.go:217 +0x13a
github.com/skelterjohn/go.wde/win.(*Window).FlushImage(0xc04206a280, 0x0, 0x0, 0x0)
        C:/src/go/src/github.com/skelterjohn/go.wde/win/win_windows.go:176 +0x1b4
github.com/sqweek/goui/wdedrv.WdeDrv.Flip(0x562b80, 0xc04206a280)
        C:/code/go/src/github.com/sqweek/goui/wdedrv/wdedrv.go:22 +0x53
github.com/sqweek/goui/wdedrv.(*WdeDrv).Flip(0xc0420080b0)
        <autogenerated>:2 +0x5d
github.com/sqweek/goui.(*Painter).Loop(0xc0420740a0)
        C:/code/go/src/github.com/sqweek/goui/paint.go:70 +0xf9
created by main.wdemain
        C:/code/go/src/github.com/sqweek/goui/example/worms/worms.go:99 +0x2d3

It's index 0:

gdi32.go:471:                 uintptr(unsafe.Pointer(&lpvBits[0])),

lpvBits corresponds to buffer.Pix in Window.blitImage, indicating that we have a zero-length pixel array. FlushImage tries to avoid this scenario (since #46):

func (this *Window) FlushImage(bounds ...image.Rectangle) {
        if this.buffer.Bounds().Empty() {
                return // happens when window is minimised
        }

So I guess this.buffer must be changing concurrently. I'm calling FlushImage from a different goroutine than the one handling EventChan but I think it's racy even without that since Window.buffer is changed when WndProc handles a WM_SIZE message, before the ResizeEvent is posted. Even the EventChan thread may observe the change to buffer before seeing the ResizeEvent.

[kirillDanshin]

I had issue like that with passing uintptr to slice data when I was working on my go-powered hypervisor. So I think that

uintptr(unsafe.Pointer(&lpvBits[0]))

also could be changed to:

uintptr(([3]unsafe.Pointer)(unsafe.Pointer(&lpvBits))[0])

so we resolve the array field instead of first slice index

[sqweek]

Hrm, thanks. Can you elaborate on the difference between the two and why it caused problems? Logically they appear to represent the same thing to me, and a quick test gives me the same value for both:

package main

import "unsafe"

func main() {
        a := make([]int, 5)
        println(uintptr(unsafe.Pointer(&a[0])))
        println(uintptr((*[3]unsafe.Pointer)(unsafe.Pointer(&a))[0]))
}

(this is all something of a tangent since this cast happens in the AllenDang/w32 package rather than go.wde)

[kirillDanshin]

sorry for the delay

basically, in normal case it should give the same value, but my way to do that will work even if

package main

import "unsafe"

func main() {
        a := make([]int, 0)
        // println(uintptr(unsafe.Pointer(&a[0]))) // uncomment to break the program
        println(uintptr((*[3]unsafe.Pointer)(unsafe.Pointer(&a))[0])) // works like a charm
}

let's look into evaluation stages to see the difference:

1. Resolve first index and get the pointer:

   println(uintptr(unsafe.Pointer(&a[0])))

stage 1. breaks if a.array len == 0.

println(uintptr(unsafe.Pointer(&a.array[0])))

stage 2. optimize, round 1

println(uintptr(unsafe.Pointer(a.array)))

stage 3. opimize, round 2. unnecessary unsafe.Pointer to unsafe.Pointer conversion

println(uintptr(a.array))

---

2. Resolve field instead of first array index

   println(uintptr((*[3]unsafe.Pointer)(unsafe.Pointer(&a))[0]))

let's just make it simple to read

package main

import (
    "unsafe"

    "github.com/gramework/runtimer"
)

func main() {
    a := make([]int, 0)
    println(uintptr((*runtimer.SliceType2)(unsafe.Pointer(&a)).Array))
}

split on stages:

package main

import (
    "unsafe"

    "github.com/gramework/runtimer"
)

func main() {
    a := make([]int, 0)
    aPtr := unsafe.Pointer(&a)
    runtimeSlice := (*runtimer.SliceType2)(aPtr)
    aArrayPtr := runtimeSlice.Array
    println(uintptr(aArrayPtr))
}

[sqweek]

Ah I see... I guess the Array field resolves to nil in the zero length slice case? Anyway I think we should just avoid calling SetDIBitsToDevice with an empty slice here, but thanks for the details!

[kirillDanshin]

yep