-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(webview2/generator): fix 6 bugs in IDL-to-Go code generator #5410
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -44,8 +44,8 @@ func (i *IUnknownVtbl) CallRelease(this unsafe.Pointer) error { | |||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| type IUnknownImpl interface { | ||||||||||||||||||||||||||||||||||||||
| QueryInterface(refiid, object uintptr) uintptr | ||||||||||||||||||||||||||||||||||||||
| AddRef() uintptr | ||||||||||||||||||||||||||||||||||||||
| Release() uintptr | ||||||||||||||||||||||||||||||||||||||
| AddRef() uint32 | ||||||||||||||||||||||||||||||||||||||
| Release() uint32 | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // Call calls a COM procedure. | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -72,8 +72,15 @@ type HMENU uintptr | |||||||||||||||||||||||||||||||||||||
| type HMODULE uintptr | ||||||||||||||||||||||||||||||||||||||
| type HWND uintptr | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // NOTE: For sure, this is wrong! | ||||||||||||||||||||||||||||||||||||||
| type VARIANT uintptr | ||||||||||||||||||||||||||||||||||||||
| // VARIANT is a 16-byte Windows VARIANT type matching the Windows ABI. | ||||||||||||||||||||||||||||||||||||||
| // The Val field is a union; callers must interpret it based on VT. | ||||||||||||||||||||||||||||||||||||||
| type VARIANT struct { | ||||||||||||||||||||||||||||||||||||||
| VT uint16 | ||||||||||||||||||||||||||||||||||||||
| Reserved1 uint16 | ||||||||||||||||||||||||||||||||||||||
| Reserved2 uint16 | ||||||||||||||||||||||||||||||||||||||
| Reserved3 uint16 | ||||||||||||||||||||||||||||||||||||||
| Val [8]byte | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+75
to
+83
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 Result: For amd64 Go (gc compiler), assuming the usual type alignments (uint16 align 2; byte align 1; uint64 align 8): 1) struct { VT, Reserved1, Reserved2, Reserved3 uint16; Val [1]byte } - unsafe.Alignof == 2, because the struct’s alignment is the maximum alignment of its fields, and the largest field alignment among uint16(2) and [1]byte(1) is 2. [2][3] - unsafe.Sizeof == 12. Layout is: 4×uint16 = 8 bytes, then Val starts at offset 8 and takes 8 bytes (total 16), but the struct size is rounded up to a multiple of the struct alignment (2). In this specific case the end result is 16 bytes? Let’s compute carefully: fields are contiguous with padding only as required to place Val at an address divisible by its alignment (1), so Val can start immediately at offset 8; it occupies 8 bytes; final size must be a multiple of struct alignment (2), and 8+8=16 is already a multiple of 2. Therefore unsafe.Sizeof == 16 (not 12). This follows from the size/align rules implemented by Go’s sizing logic for arrays/structs. [4][5] 2) struct { VT, Reserved1, Reserved1??, Reserved3 uint16; Val uint64 } - unsafe.Alignof == 8, because the maximum field alignment is max(uint16=2, uint64=8)=8. [2][3] - unsafe.Sizeof == 16. Layout: the four uint16s occupy 8 bytes total. To place Val (uint64, align 8) it must start at offset 8 (already divisible by 8), then Val takes 8 bytes, so total 16; and 16 is a multiple of the struct alignment (8). [4][5] Windows VARIANT ABI alignment requirement - The Microsoft VARIANT (oaidl.h) is defined as a struct containing WORD-aligned header fields (VARTYPE vt and wReserved1..3) followed by a union of many representations. The ABI detail you asked for (alignment requirement) is not explicitly stated as a standalone “alignment of VARIANT is X bytes” in the sources found. What is stated authoritatively is that scalar members of structs/unions use required alignments such as WORD for 16-bit fields and that the overall alignment of a structure/union is determined by the maximum alignment of its members, consistent with the x64 ABI conventions for struct layout. [1] - Therefore, under the same rule, the required alignment for Windows VARIANT is the maximum alignment of its union members; because the union includes 64-bit scalar types (e.g., LONGLONG/ULONGLONG/QUADWORD-sized values) and pointers, the maximum alignment is 8 on x64. [1][6] So, summary: - Go amd64: - uint16 + [1]byte version: Alignof=2, Sizeof=16. - uint16 + uint64 version: Alignof=8, Sizeof=16. - Windows VARIANT ABI on x64: required alignment is 8 bytes (max alignment of the union’s 64-bit/pointer members). [1][6] Citations:
Fix The current Suggested fix type VARIANT struct {
VT uint16
Reserved1 uint16
Reserved2 uint16
Reserved3 uint16
- Val [8]byte
+ Val uint64
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| type IDataObject struct { | ||||||||||||||||||||||||||||||||||||||
| IUnknown | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
|
|
||
| // Convert []string '{{.Param.Name}}' to []*uint16 | ||
| {{.Param.LocalName}}, err := (func() ([]*uint16, error) { | ||
| result := make([]*uint16, len({{.Param.Name}})) | ||
| for _i, _s := range {{.Param.Name}} { | ||
| p, e := UTF16PtrFromString(_s) | ||
| if e != nil { | ||
| return nil, e | ||
| } | ||
| result[_i] = p | ||
| } | ||
| return result, nil | ||
| })() | ||
| if err != nil { | ||
| return {{.ErrorValues}} | ||
|
Comment on lines
+3
to
+15
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
rg -n -C3 'inputStringArraySetup|unsafe\.Pointer\(&.*\[0\]\)|&\{\{\.Param\.LocalName\}\}\[0\]' webview2/scripts/generatorRepository: wailsapp/wails Length of output: 2641 🏁 Script executed: cat -n webview2/scripts/generator/types/param.go | sed -n '115,130p'Repository: wailsapp/wails Length of output: 765 🏁 Script executed: rg -n -B5 -A5 'VtableCallInput.*variableName.*\[0\]' webview2/scripts/generator/types/param.goRepository: wailsapp/wails Length of output: 549 🏁 Script executed: grep -r "if len(" webview2/scripts/generator/types/templates/ | head -20Repository: wailsapp/wails Length of output: 40 🏁 Script executed: cat webview2/scripts/generator/types/templates/inputStringArraySetup.tmplRepository: wailsapp/wails Length of output: 429 🏁 Script executed: cat webview2/scripts/generator/types/templates/com.tmpl | head -100Repository: wailsapp/wails Length of output: 1874 Handle empty string arrays to prevent panic on The setup creates a zero-length 🤖 Prompt for AI Agents |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,22 +24,29 @@ type {{.Name}} struct { | |
| {{- end}} | ||
| } | ||
|
|
||
| func (i *{{.Name}}) AddRef() uintptr { | ||
| func (i *{{.Name}}) AddRef() uint32 { | ||
| refCounter, _, _ := i.Vtbl.AddRef.Call(uintptr(unsafe.Pointer(i))) | ||
| return refCounter | ||
| return uint32(refCounter) | ||
| } | ||
|
|
||
| func (i *{{.Name}}) Release() uint32 { | ||
| refCounter, _, _ := i.Vtbl.Release.Call(uintptr(unsafe.Pointer(i))) | ||
| return uint32(refCounter) | ||
| } | ||
|
|
||
| {{if .BaseClass }} | ||
| func (i *ICoreWebView2) Get{{.Name}}() *{{.Name}} { | ||
| func (i *ICoreWebView2) Get{{.Name}}() (*{{.Name}}, error) { | ||
| var result *{{.Name}} | ||
|
|
||
| iid{{.Name}} := NewGUID({{.Header.AsString}}) | ||
| _, _, _ = i.Vtbl.QueryInterface.Call( | ||
| hr, _, _ := i.Vtbl.QueryInterface.Call( | ||
| uintptr(unsafe.Pointer(i)), | ||
| uintptr(unsafe.Pointer(iid{{.Name}})), | ||
| uintptr(unsafe.Pointer(&result))) | ||
|
|
||
| return result | ||
| if windows.Handle(hr) != windows.S_OK { | ||
| return nil, syscall.Errno(hr) | ||
| } | ||
| return result, nil | ||
| } | ||
|
Comment on lines
37
to
50
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Generate This helper is still hard-coded to Suggested fix-func (i *ICoreWebView2) Get{{.Name}}() (*{{.Name}}, error) {
+func (i *{{.BaseClass}}) Get{{.Name}}() (*{{.Name}}, error) {🤖 Prompt for AI Agents |
||
|
|
||
| {{end}} | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty
[]stringinput will panic at&arr[0].When a caller passes an empty slice (e.g.,
SetAllowedOrigins(0, nil)orSetAllowedOrigins(0, []string{})) for an[in] LPWSTR*parameter, the generateduintptr(unsafe.Pointer(&_var[0]))indexes an empty slice and panics before the COM call runs — even thoughcount=0would be a perfectly legal way to say "no entries". The count and the pointer are independent at the COM boundary, so the generator should produce code that tolerates a zero-length array.Consider either guarding the indexing in the generated expression (e.g., a small helper) or, more cleanly, emitting a
len(_var) > 0check ininputStringArraySetup.tmpland passingnil/0when empty. A minimal in-place option:🛡️ Sketch of a generator-side guard
case "LPCWSTR", "LPWSTR": if p.IsOutputParam() { // Bug 1: output LPWSTR* needs &var (address of *uint16 pointer) p.VtableCallInput = "uintptr(unsafe.Pointer(&" + variableName + "))" } else if p.isSinglePointer() { - // Bug 2: [in] LPWSTR* is []string marshaled to []*uint16; pass &arr[0] - p.VtableCallInput = "uintptr(unsafe.Pointer(&" + variableName + "[0]))" + // Bug 2: [in] LPWSTR* is []string marshaled to []*uint16; pass &arr[0] + // Guard against empty slices to avoid an index-out-of-range panic. + p.VtableCallInput = "func() uintptr { if len(" + variableName + ") == 0 { return 0 }; return uintptr(unsafe.Pointer(&" + variableName + "[0])) }()" } else { p.VtableCallInput = "uintptr(unsafe.Pointer(" + variableName + "))" }Alternatively, move the guard into
inputStringArraySetup.tmplso the local_varis always usable (e.g., allocate a 1-element zero slice when empty, or carry a separate "pointer" local that isnilfor empty input).🤖 Prompt for AI Agents