fix(webview2): fixed the generator, not its output — nine COM marshalling families - #37
Open
13thgoutham wants to merge 3 commits into
Open
fix(webview2): fixed the generator, not its output — nine COM marshalling families#3713thgoutham wants to merge 3 commits into
13thgoutham wants to merge 3 commits into
Conversation
The committed pkg/webview2 is not the output of the committed generator. Defects
here have been fixed by sweeping the 306 generated files -- "fix: com error
handling" and "fix: mischanged error values" between them touched 800+ call sites
-- and never the templates that produce them, so every regeneration silently
reverts every fix. Regenerating from the pinned IDL to check brings back 170
changed files.
Nine families, all sharing the failure mode that is why none was caught by review:
the wrong code compiles, links, runs and returns S_OK. Counts are call sites in
WebView2.1.0.2903.40.
1. Vtable inheritance, 88 interfaces. A COM vtable is flat: a derived interface's
vtable begins with its ENTIRE base chain, 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 the chain above it declares.
ICoreWebView2_14.AddServerCertificateErrorDetected dispatched at slot 4 --
ICoreWebView2::get_Settings -- instead of 107. get_Settings has one
out-parameter, so it wrote the Settings pointer over the caller's event-handler
struct and returned S_OK: registration "succeeded", registered nothing, and the
event never fired. The IDL states every base and the parser already captured it;
it was simply never used. Base-interface calls were always correct, which is why
Navigate and AddNavigationCompleted worked throughout and hid this.
2. By-value in-parameters, ~166 sites. The type switch compared p.Type -- the IDL
type, "BOOL", "INT32", "double" -- against Go type names in lower case, so it
matched almost nothing and every by-value argument fell through to the &address
catch-all, making the callee read a pointer as an integer. Includes all 61
remove_* event tokens: EventRegistrationToken is struct{int64}, so each passed
the ADDRESS of the token it was meant to match, and no event handler could ever
be removed -- remove_ still returned S_OK.
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 by address. So POINT (8) and RECT (16) take opposite forms, which is how
one &address default came to look plausible. maps.go now classifies each type,
and the generator fails on a type it has not been told about 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 gave the callee a null to write
through, so every string getter returned "" with S_OK.
4. QueryInterface accessors, 56 of 82. QueryInterface asks an OBJECT for another of
its interfaces, so an accessor belongs on an interface of the object that can
answer -- which the declared chain's ROOT names. All were emitted on
ICoreWebView2. For the ICoreWebView2_N chain that is right, since it is one
object and a caller should not walk thirteen accessors to reach _14. For every
other chain it is useless: GetICoreWebView2Controller2 hung off ICoreWebView2, a
different object, so it could only fail, while ICoreWebView2Controller had no
accessor at all. The 26 on ICoreWebView2 stay, so no existing caller breaks.
5. ComProc.Call's Errno returned as error. Call's third result is a syscall.Errno
that is NON-NIL on success ("The operation completed successfully"), so every
successful call looked like a failure. HRESULT is the status and is already
checked. That sweep also over-applied in two places, replacing
UTF16PtrFromString's genuine error with nil in GetHeader. IUnknown::Release's
refcount was discarded the same way, so CallRelease returns uint32.
6. Float in-parameters, 12 methods. Each passed the ADDRESS of a Go float64 where
the callee reads a register, so a zoom factor arrived as a denormal or ~1e-300.
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. windows/arm64 remains unsolved:
sys_windows_arm64.s loads R0-R7 and never V0-V7, carrying a TODO to do what
amd64 does. Passing bits in an integer register is no worse there than passing a
pointer was, so this is a strict improvement on both.
7. 32-bit out-parameters, 7 sites. INT/UINT mapped to Go's 64-bit int/uint, and
lowercase "int" -- the IDL's spelling for six of the seven -- was absent from the
map entirely. A 4-byte write into an 8-byte zeroed local never sign-extends, so
GetExitCode returned 3221225477 for -1073741819 (STATUS_ACCESS_VIOLATION), and
GetKeyEventLParam is wrong on every key-up because WM_KEYUP sets lParam bit 31.
8. Struct field widths. A struct field's BOOL is 4 bytes and was generated as Go's
1-byte bool, making COREWEBVIEW2_PHYSICAL_KEY_STATUS 12 bytes against a native
24. Its only use is GetPhysicalKeyStatus, which hands WebView2 the address of
that local -- so every call wrote 12 bytes past the end of a heap object, and
the last three flags, at native offsets 12/16/20, were read from padding and so
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.
9. Callback parameter widths, 3 sites. syscall.NewCallback rejects any argument
wider than a uintptr, and it does so when the callback is CONSTRUCTED -- which
happens in a package-level var initialiser. Three CompletedHandlers declared
their LPCWSTR result as a Go string (a 16-byte header), so merely importing
pkg/webview2 panicked before main() whether or not the program used them:
"compileCallback: argument size is larger than uintptr". This is wailsapp#36.
Two more, found while checking the above: an enumerator with no initialiser is
PREVIOUS + 1 in C, not its ordinal position ("A = 5, B" produced 1, not 6 -- values
are computed numerically now, so output for every shipped enum is unchanged); and
Release was generated for no interface while AddRef was generated for all 252, so
every accessor call leaked a reference with nothing to call.
Tooling, because none of this was checkable before:
scripts/regen regenerate from a pinned IDL into a chosen directory and
diff it. update_version_mapping.go only regenerates as a
side effect of finding a NEW version upstream, needs the
network, and rewrites the tree in place -- which is why the
drift went unnoticed.
gofmt in the generator
the committed tree is gofmt-clean while the generator wrote
raw template output, so someone was formatting 306 files by
hand after every regeneration. ~180 of them differed from a
fresh generation by import order alone: enough noise to hide
a real change in a regeneration diff, which is how
hand-patched output survived. Its error path also turns a
template that emits invalid Go into a named failure.
go test -update the goldens were refreshed by uncommenting an os.WriteFile
loop in seven files, which is enough friction to make
hand-editing output look like the cheaper fix.
Also: errors are returned rather than log.Fatalf, because Fatalf calls os.Exit and
this code runs inside the generator's own tests -- a reintroduced bug killed the
test binary mid-run, with no attributable failure and every later test silently
never running. And Taskfile.yml's manual gofmt over pkg/webview2 is gone.
Deliberately not fixed: array-valued parameters have no representation at all
(GetAllowedOrigins returns *string for an LPWSTR** array, so dereferencing it is an
unbounded read). Fixing that needs a slice concept and an ownership decision, which
is an API call rather than a marshalling one -- arguably those four methods should
not be emitted until then.
Marshalling bugs here are invisible to review and to go build -- the wrong conversion compiles, links, runs and returns S_OK -- so the only way to know is to execute it. 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 that makes this practical: it needs no WebView2 Runtime, no Edge, no display, no network and no elevation. Windows and nothing else, so windows-latest is enough -- and `go test -c` produces a self-contained binary you can copy to any Windows box and run. Ten tests, one per argument shape, each asserting what the pre-fix code got wrong: BOOL arrives as 0/1 rather than an address; an event token 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; an int32 out-parameter keeps its sign; a string out-parameter is given somewhere to write; a string in-parameter arrives as the *uint16 we converted; COREWEBVIEW2_PHYSICAL_KEY_STATUS is 24 bytes with its flags at the native offsets; HWND arrives by value; and the HRESULT decides the error, with S_OK yielding a nil one. Run on Windows 11, all ten pass. The generator side gains property tests over the whole surface rather than goldens for one interface -- every derived vtable embeds its declared base; every 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 as an error; and the one nothing checked before, that the committed pkg/webview2 IS 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. A GitHub Actions workflow that runs all of this -- the marshalling tests on windows-latest, and the generator's tests plus a cross-compile for windows/amd64 and arm64 on ubuntu-latest -- is ready and deliberately left out of this PR, since a workflow is a maintainer's call rather than a contributor's. Say the word and I will add it.
cd scripts && go run ./regen -idl WebView2.1.0.2903.40.idl -out ../pkg/webview2 No hand edits, and TestCommittedOutputMatchesGenerator now enforces that. Verified: windows amd64, arm64 and 386 build; go vet clean on pkg/webview2; the generator's tests pass; all six IDLs in scripts/ regenerate and the three older ones type-check; the ten marshalling tests pass on Windows 11 hardware; and a downstream consumer's Windows GUI -- window, TLS certificate pin firing, both external-link layers -- runs against this tree on a real machine. One property worth stating, because it is what makes a 170-file diff trustworthy: this output is byte-identical whether the generator runs on top of this branch's base or on top of 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 instead.
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
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. #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 #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 #36: this fixes that bug at its root (family 9) but does not contain #36's commit, so the two do not conflict. #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.