fix(webview2): fix the generator, not its output — nine COM marshalling families - #1
Merged
Merged
Conversation
Every generator test carried a commented-out os.WriteFile loop for refreshing
its golden, so updating them meant an edit-run-revert cycle across seven files.
That is enough friction to make hand-editing the generated output look like the
cheaper way to fix a bug -- and it is not, because the next regeneration
silently reverts it.
go test ./generator -update # then read the diff, then run without it
The goldens are embedded with go:embed, so a run that rewrites them is still
comparing against the previous build's copies; -update therefore writes and
returns rather than asserting.
update_version_mapping.go can only regenerate as a side effect of checking
Microsoft's release-notes page for a NEW version: it needs the network, it
rewrites the tree in place, and it does nothing at all if the pinned version is
current. None of that suits the one thing a generator change needs, which is to
regenerate from a pinned IDL and diff the result against the committed files.
go run ./regen -idl WebView2.1.0.2903.40.idl -out /tmp/baseline
diff -r /tmp/baseline ../pkg/webview2
Doing this is how the drift between the committed generator and the committed
output became measurable rather than suspected.
The generator wrote raw template output, yet the committed pkg/webview2 is gofmt-clean -- so its 306 files were being formatted by hand after every regeneration. The measurable cost: ~180 of them differ from a fresh generation by nothing but import order and blank lines, which is more than enough noise to hide a real change in a regeneration diff. Hiding real changes in regeneration diffs is how this package came to carry 800+ hand-patched call sites. Second benefit, unlooked-for: format.Source rejects invalid Go, so a template that emits something unparseable now fails the generator and names the file, instead of writing it out to fail later at go build. Only the goldens' formatting changes here; no generated logic moves.
… them
Five families, all in the same two places -- Param.processVtableCallInput and the
method template -- and all with the same failure mode: the generated code
compiles, links, runs, and returns S_OK while corrupting exactly one argument.
Counts are call sites in the pinned 1.0.2903.40 IDL.
1. By-value scalars, ~90 sites. The type switch tested p.Type, the IDL type
("BOOL", "INT32", "double"), against Go type names in lower case. It
therefore matched almost nothing, and every by-value in-parameter fell
through to the &address catch-all, so the callee read a pointer as an
integer. p.GoType is the mapped Go type and is what the comparisons meant.
One wrong field name; no scalar in-parameter in the whole binding was passed
correctly. Since Go forbids uintptr(someBool), BOOL now goes through a
boolToUintptr helper -- which subsumes the hand-written branch added for
PutShouldDetectMonitorScaleChanges, one of the 39 bool sites.
That same missing pointer guard also broke four OUT parameters, because "int"
is spelled the same in the IDL and in Go: GetStatusCode and GetExitCode
passed the value of an uninitialised local where the callee wanted somewhere
to write, so they returned 0 or an error every time.
2. Handle typedefs and register-sized aggregates, 76 sites. These reached the
&address catch-all too. The Win32 x64 rule is not "aggregates go by
reference": an aggregate of exactly 1, 2, 4 or 8 bytes is passed IN A
REGISTER as an integer of that width, and only the rest go by address. So
POINT (8 bytes) and RECT (16) take opposite forms, which is what made a
single &address default look plausible.
EventRegistrationToken is 61 of the 76. It is struct{ int64 }, so every
remove_* method in the binding handed the callee the ADDRESS of the token it
was supposed to match -- no event handler could ever be removed, and remove_
still returned S_OK.
maps.go now classifies each type explicitly and the generator FAILS on a type
in neither table, rather than guessing. Guessing is what produced all of the
above.
3. String out-parameters, 109 sites. LPWSTR out-params are declared LPWSTR*: the
callee writes a string pointer into storage we own, so it needs the address
of our local *uint16. Passing the local's nil value instead gave the callee a
null to write through, so every string getter returned "" -- with S_OK.
4. ComProc.Call's Errno returned as error. Call's third result is a
syscall.Errno, which is NON-NIL on success ("The operation completed
successfully"), so returning it made every successful call look like a
failure. HRESULT is the real status and is already checked. Upstream fixed
this across the output by hand in "fix: com error handling" and "fix:
mischanged error values" but never in the template, so any regeneration
reintroduced it; that sweep also over-applied in two places, replacing
UTF16PtrFromString's genuine error with nil in GetHeader. Both are correct
now. IUnknown::Release's refcount was being discarded the same way, and
CallRelease returns uint32 accordingly.
5. Callback parameter widths, 3 sites -- the family PR wailsapp#36 patched in the
output. A callback reached through syscall.NewCallback may not declare a
parameter wider than a uintptr, and NewCallback enforces that when the
callback is CONSTRUCTED, which happens in a package-level var initialiser. So
three CompletedHandlers declaring their LPCWSTR result as a Go string (a
16-byte header) made merely importing pkg/webview2 panic during init on
amd64, whether or not the program used them:
panic: compileCallback: argument size is larger than uintptr
An outbound method may legitimately take a string, because it converts before
the call; an inbound one has no such step, so Param.AsCallbackType keeps the
two apart.
Deliberately not fixed: float in-parameters. On Windows x64 a floating-point
argument goes in XMM0-XMM3 and syscall.LazyProc.Call fills only the integer
registers, so uintptr(f) truncates and &f is equally wrong. There is no
marshalling answer; it needs a different call mechanism. PutZoomFactor is the
live example, and the code says so rather than pretending.
Also still wrong, and out of scope: array-valued parameters have no
representation here at all. GetAllowedOrigins returns *string for an LPWSTR**
array and SetAllowedOrigins takes one string for an array, and
Get/SetCustomSchemeRegistrations has the same shape. Fixing that needs a slice
concept and an ownership decision, not a marshalling change.
A COM vtable is flat: a derived interface's vtable begins with its ENTIRE base chain and only then its own methods. Every derived vtable was generated as IUnknownVtbl plus that interface's own methods, so each method sat too early by however many methods the chain above it declares, and a call landed on whichever unrelated function occupies that offset. It cannot fail loudly -- the wrong slot holds a real function pointer, so it runs and returns S_OK. ICoreWebView2_14's AddServerCertificateErrorDetected dispatched at slot 4, which is ICoreWebView2::get_Settings. get_Settings takes one out-parameter, so it duly wrote the Settings pointer over the caller's event-handler struct and reported success. Its correct slot is 107. Registration therefore "succeeded", registered nothing, and the event never fired -- which on our side meant a TLS certificate pin that could not engage on any Windows machine. 88 interfaces were affected. Base-interface calls were always correct, which is exactly why Navigate and AddNavigationCompleted worked throughout and hid it. The IDL states the base for every interface and the parser already captured it; it was simply never used here. A first-generation interface's base IS IUnknown, so one expression covers both cases -- and taking the base from the IDL rather than from the version suffix matters: ICoreWebView2EnvironmentOptions2 through 8 each derive from IUnknown, not from their predecessor, so inferring the chain from the numbering gets those seven wrong. Second, related: the QueryInterface accessors. QueryInterface asks an OBJECT for another of its interfaces, so an accessor belongs on an interface of the object that can answer it -- which the declared chain's ROOT names, not the immediate base. The template put all of them on ICoreWebView2. For the ICoreWebView2_N chain that is correct, because it is all one object and a caller should not have to walk thirteen accessors to reach _14. For every other chain it is useless: GetICoreWebView2Controller2 hung off ICoreWebView2, which is a different object and can only fail, while ICoreWebView2Controller, which can answer, had no accessor at all. So of the 82 accessors, 56 now root on their own object (Environment, Settings, Profile, Frame, Controller, CompositionController and the event-args chains) and 26 stay on ICoreWebView2. 31 of the 56 were already correct in the committed output because they had been moved by hand, so a regeneration relocates the remaining 25 -- and breaks no existing caller. An earlier draft of this rooted on the immediate base instead. That is also sound COM, but it forces the walk and it did break callers of GetICoreWebView2_14, including ours. Interfaces whose base is IUnknown keep their ICoreWebView2 receiver: there is no sibling interface to reach them from, and re-rooting them onto IUnknown would move ~170 methods onto it and delete accessors that already ship, which is an API break rather than a bug fix.
Six properties of the whole generated binding, checked against the pinned IDL,
rather than goldens for one interface. Every one of them fails on main and passes
after this series.
A golden records what one interface looked like on the day it was written. These
say what must be true of all 306 files, which is the assertion that was missing:
each fixed family was wrong across dozens of interfaces while the golden for
ICoreWebView2_3 sat there passing.
TestVtableEmbedsDeclaredBase every derived vtable embeds its
declared base's vtable, so no method
can sit at the wrong slot
TestQueryInterfaceAccessorReceiver accessors hang off the object that can
answer the QueryInterface, i.e. the
declared chain's root
TestCallbackParamsFitInAUintptr no callback declares a parameter wider
than a uintptr -- i.e. the package can
be imported at all
TestCallErrnoIsNeverReturnedAsError Call's Errno is never returned as error
TestByValueArgumentsAreNotPassedByAddress table-driven per parameter shape: BOOL,
UINT32, HWND, 8-byte token, POINT,
COLOR, RECT, in/out pointers, in/out
strings
TestGeneratedOutputIsFormatted the output is a gofmt fixpoint
The vtable test asserts embedding rather than computed slot numbers on purpose:
embedding the immediate base is sufficient by induction, whereas a test that
recomputed the offsets would be checking its own arithmetic against itself.
cd scripts && go run ./regen -idl WebView2.1.0.2903.40.idl -out ../pkg/webview2
No hand edits. That is the point of the commit: before this series the committed
pkg/webview2 was NOT the output of the committed generator, so a regeneration
would have reverted 800+ corrections that only ever existed in the output.
108 files change, and every changed line belongs to a family fixed earlier in the
series:
61 EventRegistrationToken passed by value (32 files) remove_* now works
54 BOOL passed by value (36 files)
51 handle typedefs and integers by value (25 files)
25 QI accessors rooted on their own object (25 files)
9 POINT passed by value (4 files)
8 vtable embeds its declared base (8 files)
6 int out-parameters passed by address (6 files) Get{Status,Exit}Code
4 UTF16PtrFromString's error kept (2 files) GetHeader
Verified: GOOS=windows GOARCH=amd64 go build ./... and GOARCH=arm64
go build ./pkg/..., go vet ./pkg/webview2 clean, the generator's own tests pass,
and the Zentinel Windows GUI that consumes this package builds against this tree
unchanged -- checked through a go.work override, since a pseudo-version in go.mod
silently resolves to the module cache instead.
Not verified: none of this has been exercised on Windows hardware in this exact
form. The vtable, cert-pin and external-link paths were verified on a real machine
against the equivalent hand-patched tree, which is where these fixes came from.
The by-value corrections -- remove_*, POINT, the handle typedefs -- are new, and
so far rest on the calling convention rather than on a run.
Reviewed by three independent passes over the preceding seven commits. One commit
rather than several because the fixes overlap in the same two functions, and
file-level boundaries would have misrepresented which change belongs where.
## Regressions this series introduced
1. `defaultErrorValue` tested `uintptrTypedef[p.Type]`, and `p.Type` carries no
indirection -- so `[out] HANDLE**` returned the integer 0 for a `*HANDLE`,
which does not compile. The `p.GoType == "HANDLE"` test it replaced was false
for a double pointer and fell through to `nil`. The pointer case now comes
first, which is the right order on its own terms: for `*T` the only thing that
matters is that it is a pointer.
2. `Library.Process` pre-indexes interfaces with the comment "nothing guarantees
a base is declared first", but enums were still registered as each one was
processed, while `Param.IsEnum()` is consulted while processing an INTERFACE.
An enum declared after its user therefore looked like an unknown type -- which
used to mean a silently wrong &address and, after this series, means the
generator stops with advice that does not apply. Same bug class, half fixed.
Both are indexed in one pre-pass now.
## Claims this series made that were false
3. "Interfaces whose base is IUnknown keep the ICoreWebView2 receiver they have
today" -- they do not, and never did. The accessor is guarded by `BaseClass`,
which `generateVtbl` blanks for `IUnknown`, so those 169 interfaces get no
accessor at all. The `{{else}}` arm that supposedly served them was
unreachable, as was the matching fallback in the test. The statement appeared
in three places including a test comment; all three now say what happens.
4. "Float in-parameters have no marshalling answer; LazyProc.Call can only fill
the integer registers." Wrong. `runtime/sys_windows_amd64.s` copies each of
the first four argument slots into the matching XMM register, with a comment
saying it does so precisely "in case any of the arguments are floating point
values". So the bit pattern IS the argument, and `math.Float64bits` produces
it. The scope was understated too: 12 methods take a `double`, not one, and
every one was handing the callee the ADDRESS of a Go float reinterpreted as a
double -- so a zoom factor was a denormal or ~1e-300.
windows/arm64 is genuinely unsolved: `sys_windows_arm64.s` loads R0-R7 and
never V0-V7, with a TODO to do what amd64 does. Passing the bits in an integer
register is no worse there than passing a pointer was, so this is a strict
improvement on both, and the comment now says which arch is which.
## Gaps in the families this series claimed to fix
5. The `*` and LPWSTR branches gained a direction check and the `**` branch did
not. An IN parameter is already the `T**` the caller built, so taking its
address hands the callee a `T***`: it reads our local's own value as the first
element and calls through it. `CreateObjectCollection` is the live example, and
it is a wild call rather than a wrong value.
6. `INT`/`UINT`/`int` mapped to Go's `int`/`uint`, which are 64-bit. Seven
out-parameters declare a local of the mapped type and hand over its address,
so a 64-bit local took a 32-bit write and kept its zeroed high half: the sign
never extended. `GetExitCode` returned 3221225477 for an exit code of
-1073741819, and `GetKeyEventLParam` is wrong on every key-up, because WM_KEYUP
sets lParam bit 31. Lowercase `int` -- the IDL's spelling for six of the seven
-- was absent from the map entirely. Seven public signatures change from
`int`/`uint` to `int32`/`uint32`; they returned wrong values, so there is
nothing to preserve.
## Pre-existing defects found on the way
7. A struct field's `BOOL` is 4 bytes and was generated as Go's 1-byte `bool`, so
`COREWEBVIEW2_PHYSICAL_KEY_STATUS` came out 12 bytes against a native 24. Its
only use is `GetPhysicalKeyStatus`, which hands WebView2 the address of that
12-byte local -- so every call wrote 12 bytes past the end of a heap object,
into whatever shared its size class. The bytes that landed inside were misread
too: the last three flags sit at native offsets 12/16/20 while Go looked at
9/10/11, so they were permanently false. Struct fields now have their own type
map: a parameter's BOOL is converted at the boundary, so Go's `bool` is a free
kindness there, but a struct field has no boundary and its width is
load-bearing.
8. An enumerator with no initialiser is PREVIOUS + 1 in C; the generator used its
ordinal position. That is the same answer only for an enum which either sets no
values or sets them to their own positions -- true of every shipped WebView2
enum, which is why it never showed. "A = 5, B, C" produced 5, 1, 2. Values are
computed numerically where the initialiser is a literal the grammar accepts, so
the output for every shipped enum is byte-identical; where it cannot be
evaluated the previous enumerator is named instead, which Go allows.
9. `AddRef` was generated for all 252 interfaces and `Release` for none, so every
caller of an accessor leaked a reference with nothing to call. Now generated,
additively, except on handler interfaces -- those are objects we implement, so
their lifetime is the Go object's and calling through the vtable would re-enter
our own impl. Relatedly, the accessors discard the HRESULT, which cannot change
without altering 82 signatures; that is now documented where it is generated,
including that this series makes it MORE reachable, because the 56 accessors it
re-rooted were previously on an object that could never answer them.
10. `InterfaceDeclaration.Process` broke out of its loop on finding `Invoke`, so a
method declared after it was never processed and would be emitted with no
parameters. `IDL.Generate` returned inside its loop over libraries, so only
the first was ever generated. Both are unreachable with today's inputs; both
are gone.
## Tooling and tests
11. `log.Fatalf` in the generator calls `os.Exit`, and it is reachable from the
generator's own tests -- so reintroducing a bug killed the test binary mid-run
with no attributable failure, and every test after it silently never ran.
Errors are returned and reported now.
12. `TestGeneratedOutputIsFormatted` could not fail for the reason it claimed: its
input had already been through `gofmtAll`, so it asserted that formatting
formatted content is a no-op, and deliberately mangling a template left it
green. Replaced with `TestCommittedOutputMatchesGenerator`, which asserts the
one invariant this whole arrangement exists to establish and which nothing
checked -- that the committed `pkg/webview2` IS the generator's output.
Verified to go red when a committed file is hand-edited.
13. The anti-vacuity thresholds were counts of today's IDL with two units of
headroom, so one interface silently ceasing to generate a file would have been
caught by neither the skipped inner assertions nor the floor. They are exact
completeness assertions now, derived from the same rule the generator uses.
The callback-width test also missed floats, which `NewCallback` rejects with
its own panic, and the test's chain walk lacked the cycle guard the production
code had just gained.
14. `regen` did not clear its output directory, so reusing one across two IDL
versions left files only the earlier one produced and made `diff -r` read as
though they were still generated -- defeating the one thing the command is
for. `Taskfile.yml`'s `gofmt` task over `pkg/webview2` is deleted: formatting
is the generator's job as of this series, and leaving the task implied
otherwise.
## Reverted from this series
A hard failure when an interface's base is not declared in the same library. That
is a legitimate input: `com.tmpl` hand-writes `IUnknown`, `IStream` and
`IDataObject` precisely so interfaces can derive from types no IDL declares, the
IDL carries forward declarations, and this generator's own fixtures are
single-interface fragments whose base is absent by construction. The cost is that
a genuinely missing base surfaces as `undefined: <Base>Vtbl` in the consuming
build; distinguishing that from "hand-written elsewhere" needs a registry of what
com.tmpl defines, which is a worse coupling than the deferred error.
## Not fixed, deliberately
- Array-valued parameters still have no representation (`GetAllowedOrigins`
returns `*string` for an `LPWSTR**`). Dereferencing the result builds a string
header from a pointer array, so it is an unbounded read, not a wrong value. The
honest fix is to stop emitting these four methods or return an opaque type, and
both are API decisions rather than marshalling ones.
- `CoTaskMemFree` is skipped when a method returns early on a failed HRESULT. Only
a callee that allocates and then fails leaks, which is a contract violation, and
the local is nil in that case anyway.
- `!= windows.S_OK` rejects other COM success codes; `S_FALSE` appears nowhere in
any of the six IDLs.
- Enum constants are untyped, so `type X uint32` buys no type safety.
cd scripts && go run ./regen -idl WebView2.1.0.2903.40.idl -out ../pkg/webview2
No hand edits, and TestCommittedOutputMatchesGenerator now enforces that.
170 files change. The bulk is additive -- a Release method on each of the 170
non-handler interfaces -- and the rest is the corrections in the previous commit:
12 doubles passed by value rather than by address
7 32-bit out-parameters given 32-bit locals
6 COREWEBVIEW2_PHYSICAL_KEY_STATUS fields at native width (12 -> 24 bytes)
1 T** in-parameter no longer passed as T***
82 QueryInterface accessors documented as nil-on-old-runtime
Verified: windows amd64, arm64 and 386 build; go vet clean on pkg/webview2; the
generator's tests pass, including the new committed-equals-generated invariant;
all six IDLs in scripts/ regenerate and the three older ones type-check; and the
Zentinel Windows GUI and tray build against this tree through a go.work override.
Every correctness claim in this series -- mine and three reviewers' -- has been an
argument about the Windows x64 calling convention. None of it had been run. That
is precisely how eight families of marshalling bug survived here in the first
place: the wrong code compiles, links, runs and returns S_OK, so reading it was
the only check there was, and reading it is what failed.
A COM object is a pointer to a table of function pointers, so one can be built
entirely out of Go: fill a generated Vtbl with NewComProc(someGoFunc) and hand its
address to the generated wrapper. The wrapper marshals exactly as it would for
real WebView2, and the fake callee sees what actually arrived.
The part worth noticing is what this does NOT need: no WebView2 Runtime, no Edge
install, no display, no network, no elevation. Windows and nothing else -- so it
runs on a stock windows-latest runner, which is the difference between "we
reasoned carefully" and "we know". There is no CI in this repo yet; when there is,
this is the job that matters.
Nine tests, one per family, each asserting the thing the pre-fix code got wrong:
BOOL in-param arrives as 0/1, not as an address
EventRegistrationToken arrives as its 8 bytes, so remove_* can match it
POINT arrives by value with X in the low half
a double arrives as its bits, observable because Go's syscall asm copies each
of the first four argument slots into both the integer and the XMM register
an int32 out-param keeps its sign (-1073741819, not 3221225477)
a string out-param is written through OUR storage, not a null
a string in-param arrives as the *uint16 we converted
COREWEBVIEW2_PHYSICAL_KEY_STATUS is 24 bytes and its flags read at the native
offsets -- the test writes a full native-layout struct, so a regression to
the 12-byte version corrupts memory or returns false flags
HWND arrives by value
the HRESULT decides the error, and S_OK yields a nil one
Also added, in the generator's own suite: TestVtableDeclaresEveryDeclaredMethod.
Embedding the base vtable gives correct offsets only if each interface also
contributes exactly its own methods in order; the second half of that was asserted
in a commit message and never checked. If the parser ever dropped a method, every
later slot would shift and the failure would look exactly like the bug this series
fixed, while every embedding assertion still passed. Verified: 252 interfaces, one
embedded base and one ComProc per declared method, no exceptions.
Two `go vet` findings remain in the new test file, both in the CoTaskMemAlloc
helper. A string out-parameter must be freed by CoTaskMemFree, so the test has to
hand back COM-allocated memory rather than Go memory, x/sys exposes CoTaskMemFree
but no typed CoTaskMemAlloc, and a syscall returns uintptr -- so one uintptr to
unsafe.Pointer conversion is unavoidable. The memory is owned by the COM allocator
and never moves, which is why the pattern is safe; vet says "possible" because it
cannot know that. Every callback takes typed pointer arguments specifically so
that this is the only place it happens.
Writing a real string back meant CoTaskMemAlloc, because the generated cleanup frees it with CoTaskMemFree and giving that a Go pointer corrupts the COM heap. Reading a syscall's returned address back into an unsafe.Pointer is the one pattern go vet cannot verify, so the file failed the vet step -- the suite added to catch silent breakage would have arrived broken. What that family got wrong is that the callee received the local's nil VALUE instead of its address, and asserting that needs no allocation: CoTaskMemFree(nil) and UTF16PtrToString(nil) are both defined no-ops, so the path still runs end to end. The conversion is library code rather than generated code, and the real round-trip is covered in the in-parameter direction.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
pkg/webview2is unusable as shipped on amd64: importing it panics during package init. wailsapp#36 fixes that in three lines. This PR is what I found when I went looking for why.The committed
pkg/webview2is not the output of the committed generator. Defects here have been fixed by sweeping the 306 generated files —fix: com error handlingandfix: mischanged error valuesbetween them touched 800+ call sites — and never the templates that produce them. So every regeneration silently reverts every fix. I did that regeneration to check, and 174 files came back changed.So this fixes the generator and commits the regeneration. Nine families, and they share a failure mode that is worth stating plainly, because it is why none of them was ever caught by review: the wrong code compiles, links, runs and returns
S_OK.IUnknownVtblinstead of the base's vtable, so every method sat too early.ICoreWebView2_14.AddServerCertificateErrorDetecteddispatched at slot 4 —ICoreWebView2::get_Settings— instead of 107, andget_Settingshas one out-parameter, so it wrote the Settings pointer over the caller's event-handler struct and returnedS_OK"BOOL","INT32") against Go type names, so it matched almost nothing and everything fell through to&address. Includes all 61remove_*event tokens: each passed the address of the 8-byte token it was supposed to match, so no event handler could ever be removedLPWSTR*out-params passed the local's nil value rather than its address, so the callee had nowhere to write and every string getter returned""QueryInterfaceaccessorsICoreWebView2regardless of which object implements the interface, soGetICoreWebView2Controller2asked a webview for a controller interface and could only fail, whileICoreWebView2Controllerhad no accessor at allCall's Errno as errorComProc.Callis asyscall.Errnothat is non-nil on success, so every successful call looked like a failure.IUnknown::Release's refcount was discarded the same wayINT/UINT/intmapped to Go's 64-bitint/uint, so a 4-byte write left the high half zero and the sign never extended —GetExitCodereturned3221225477for-1073741819BOOLgenerated as Go's 1-bytebool, makingCOREWEBVIEW2_PHYSICAL_KEY_STATUS12 bytes against a native 24.GetPhysicalKeyStatuspasses that local's address, so every call wrote 12 bytes past the end of a heap object, and three of the four flags were read from paddingLPCWSTRresult declared as a Gostring;NewCallbackrejects anything wider than a uintptr and does so when the callback is constructed, which happens in a package-level var — hence the import-time panic in wailsapp#36Verified on Windows, not just reasoned about
This is the part I would want if I were reviewing it. Marshalling bugs are invisible to review and to
go build, so the PR adds a way to execute them.A COM object is a pointer to a table of function pointers, so one can be built entirely out of Go: fill a generated
VtblwithNewComProc(someGoFunc)and hand its address to the generated wrapper. The wrapper marshals exactly as it would for real WebView2 and the fake callee sees what actually arrived.It needs no WebView2 Runtime, no Edge, no display and no network — Windows and nothing else, so
windows-latestis enough. Ten tests, one per argument shape, all passing on a real Windows 11 machine:The generator side gains property tests over the whole surface rather than goldens for one interface — every derived vtable embeds its declared base; every interface's vtable declares exactly the methods the IDL declares (252/252, which is the half of the slot-offset argument that embedding alone does not establish); no callback parameter exceeds a register;
Call's Errno is never returned; and — the one nothing checked before — the committedpkg/webview2is the generator's output. That last test is what stops this regressing the same way again, and it is verified to go red when a generated file is hand-edited.API changes, called out rather than buried
CallReleaseandIStream.Releasereturnuint32(the refcount) instead oferror.int32/uint32instead ofint/uint. They returned wrong values before.COREWEBVIEW2_PHYSICAL_KEY_STATUS's flag fields areint32instead ofbool— required to match the native 24-byte layout.QueryInterfaceaccessors move onto the object that can answer them. TheICoreWebView2_Nfamily stays onICoreWebView2, so callers of those are unaffected.Release()is now generated alongsideAddRef()on the 170 non-handler interfaces.QueryInterfaceAddRefs, so every accessor call previously leaked a reference with nothing to call. Additive.Things left broken, deliberately
GetAllowedOriginsreturns*stringfor anLPWSTR**array, so dereferencing the result builds a Go string header out of a pointer array — an unbounded read, not a wrong value. Same shape forGet/SetCustomSchemeRegistrations. Fixing it needs a slice concept and an ownership decision, which is an API call for a maintainer rather than a marshalling fix. Arguably these four methods should not be emitted at all until then.sys_windows_arm64.sloads R0–R7 and never V0–V7, carrying a TODO to do what amd64 does. Passing the bits in an integer register is no worse there than passing a pointer was, so this is a strict improvement on both, but arm64 is not correct.CoTaskMemFreeis skipped when a method returns early on a failed HRESULT; only a callee that allocates and then fails leaks, which is a contract violation.!= windows.S_OKrejects other success codes;S_FALSEappears in none of the six IDLs inscripts/.Reviewing this
The 174 changed files under
pkg/webview2are machine output —cd scripts && go run ./regen -idl WebView2.1.0.2903.40.idl -out /tmp/x && diff -r /tmp/x ../pkg/webview2is empty. The reviewable change is the 29 files underscripts/, and the three commits separate it cleanly: the generator, then the tests, then the regeneration on its own so it can be skipped in review and reproduced instead.Worth one sanity check if you want it: that regeneration is byte-identical whether the generator runs on this base or on a tree that had all nine families patched by hand. The output is derived, so the base does not matter — which is the whole argument for fixing the generator rather than its output.
scripts/regenis new and is what makes any of this checkable:update_version_mapping.goonly regenerates as a side effect of finding a NEW version upstream, needs the network, and rewrites the tree in place, so there was no way to regenerate a pinned IDL and diff it. That is why the drift went unnoticed for so long.A GitHub Actions workflow running all of the above — the marshalling tests on
windows-latest, the generator's tests and a cross-compile onubuntu-latest— is ready and deliberately not in this PR, since adding a workflow is a maintainer's call rather than a contributor's. Say the word and I will add it.On wailsapp#36: this fixes that bug at its root (family 9) but does not contain wailsapp#36's commit, so the two do not conflict. wailsapp#36 remains the version you can merge in ten seconds if you want the package importable now and would rather take this one slowly — I would suggest exactly that.