Skip to content

fix(webview2): fixed the generator, not its output — nine COM marshalling families - #37

Open
13thgoutham wants to merge 3 commits into
wailsapp:mainfrom
13thgoutham:pr/generator-first
Open

fix(webview2): fixed the generator, not its output — nine COM marshalling families#37
13thgoutham wants to merge 3 commits into
wailsapp:mainfrom
13thgoutham:pr/generator-first

Conversation

@13thgoutham

Copy link
Copy Markdown

pkg/webview2 is 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/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. 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.

family sites what the generated code did
vtable inheritance 88 interfaces derived vtables embedded IUnknownVtbl instead of the base's vtable, so every method sat too early. ICoreWebView2_14.AddServerCertificateErrorDetected dispatched at slot 4 — ICoreWebView2::get_Settings — instead of 107, and get_Settings has one out-parameter, so it wrote the Settings pointer over the caller's event-handler struct and returned S_OK
by-value in-params ~166 the type switch compared the IDL type ("BOOL", "INT32") against Go type names, so it matched almost nothing and everything fell through to &address. Includes all 61 remove_* event tokens: each passed the address of the 8-byte token it was supposed to match, so no event handler could ever be removed
string out-params 109 LPWSTR* out-params passed the local's nil value rather than its address, so the callee had nowhere to write and every string getter returned ""
QueryInterface accessors 56 of 82 rooted on ICoreWebView2 regardless of which object implements the interface, so GetICoreWebView2Controller2 asked a webview for a controller interface and could only fail, while ICoreWebView2Controller had no accessor at all
Call's Errno as error all the third result of ComProc.Call is a syscall.Errno that is non-nil on success, so every successful call looked like a failure. IUnknown::Release's refcount was discarded the same way
float in-params 12 passed a Go float's address where the callee reads a register. Fixable on amd64: Go's syscall assembly copies each of the first four argument slots into the matching XMM register, precisely so this works
32-bit out-params 7 INT/UINT/int mapped to Go's 64-bit int/uint, so a 4-byte write left the high half zero and the sign never extended — GetExitCode returned 3221225477 for -1073741819
struct field widths 1 type a struct field's BOOL generated as Go's 1-byte bool, making COREWEBVIEW2_PHYSICAL_KEY_STATUS 12 bytes against a native 24. GetPhysicalKeyStatus passes 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 padding
callback widths 3 LPCWSTR result declared as a Go string; NewCallback rejects 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 #36

Verified 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 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.

It needs no WebView2 Runtime, no Edge, no display and no network — Windows and nothing else, so windows-latest is enough. Ten tests, one per argument shape, all passing on a real Windows 11 machine:

=== RUN   TestBoolInParamArrivesByValue
--- PASS: TestBoolInParamArrivesByValue (0.00s)
...
=== RUN   TestHResultDecidesTheError
--- PASS: TestHResultDecidesTheError (0.00s)
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 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 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.

API changes, called out rather than buried

  • CallRelease and IStream.Release return uint32 (the refcount) instead of error.
  • Seven getters return int32/uint32 instead of int/uint. They returned wrong values before.
  • COREWEBVIEW2_PHYSICAL_KEY_STATUS's flag fields are int32 instead of bool — required to match the native 24-byte layout.
  • 56 QueryInterface accessors move onto the object that can answer them. The ICoreWebView2_N family stays on ICoreWebView2, so callers of those are unaffected.
  • Release() is now generated alongside AddRef() on the 170 non-handler interfaces. QueryInterface AddRefs, so every accessor call previously leaked a reference with nothing to call. Additive.

Things left broken, deliberately

  • Array-valued parameters have no representation. GetAllowedOrigins returns *string for an LPWSTR** array, so dereferencing the result builds a Go string header out of a pointer array — an unbounded read, not a wrong value. Same shape for Get/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.
  • Float in-params on windows/arm64. sys_windows_arm64.s loads 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.
  • 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.
  • != windows.S_OK rejects other success codes; S_FALSE appears in none of the six IDLs in scripts/.

Reviewing this

The 174 changed files under pkg/webview2 are machine output — cd scripts && go run ./regen -idl WebView2.1.0.2903.40.idl -out /tmp/x && diff -r /tmp/x ../pkg/webview2 is empty. The reviewable change is the 29 files under scripts/, 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/regen is new and is what makes any of this checkable: 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, 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 on ubuntu-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.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants