Skip to content

fix(forms): ignore stale value accessor callbacks after control is rebound#69747

Open
MedOussemaNjimi wants to merge 1 commit into
angular:mainfrom
MedOussemaNjimi:fix-forms-stale-cva-callbacks
Open

fix(forms): ignore stale value accessor callbacks after control is rebound#69747
MedOussemaNjimi wants to merge 1 commit into
angular:mainfrom
MedOussemaNjimi:fix-forms-stale-cva-callbacks

Conversation

@MedOussemaNjimi

@MedOussemaNjimi MedOussemaNjimi commented Jul 12, 2026 •
edited
Loading

Copy link
Copy Markdown

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.dev application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #68744

When the FormControl bound via [formControl] (or resolved by formControlName after a
FormGroup swap) is replaced with a different instance, the previously bound control can be
silently overwritten with the new control's value.

cleanUpControl disconnects the old view-to-model pipeline by calling
valueAccessor.registerOnChange(noop) / registerOnTouched(noop), which assumes the value
accessor replaces the previously registered callback. Custom ControlValueAccessor
implementations commonly accumulate callbacks instead — e.g.:

registerOnChange(fn: any): void {
  this.innerControl.valueChanges.subscribe(fn);
}

With such an accessor, the stale callback registered for the old control stays alive. During
re-setup, setUpControlValueAccessor calls writeValue(newControl.value), the inner control
emits, the stale callback fires, and updateControl writes the new control's value into the
old control (also marking it dirty). See the minimal reproduction in the issue.

What is the new behavior?

The callbacks registered by setUpViewChangePipeline and setUpBlurPipeline now check that the
directive is still attached to the control they were created for (dir.control !== control) and
become inert otherwise. Swapping the bound control no longer corrupts the previously bound
control's value, dirty, or touched state, regardless of how the value accessor implements
registerOnChange/registerOnTouched.

Directives affected were verified for correct sequencing:

  • FormControlDirective: this.form already points at the new control when ngOnChanges runs.
  • FormControlName: _setupWithForm assigns this.control before setting up the pipelines.
  • NgModel: its control never changes instance, so behavior is unchanged.

A regression test reproducing the issue's scenario was added to reactive_integration_spec.ts
(fails without the fix, passes with it).

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

An alternative considered was to document that registerOnChange must replace previously
registered callbacks, but the accumulating pattern is widespread in existing applications and the
guard makes the cleanup mechanism robust for both semantics at the cost of a single identity
check.

Closes #68744

@izpapp
fix(forms): ignore stale value accessor callbacks after control is re…
c5ebc64 
…bound

`cleanUpControl` disconnects a directive from its previous control by
registering noop `onChange`/`onTouched` callbacks on the value accessor,
which assumes the accessor replaces previously registered callbacks.
Value accessors that accumulate callbacks instead (e.g. by subscribing
each one to an inner control's `valueChanges`) keep invoking the stale
callback, so rebinding `[formControl]` (or swapping a control inside a
`FormGroup`) overwrites the previously bound control's value with the
new control's value as soon as `writeValue` runs.

Guard the view-to-model and blur pipelines so that callbacks registered
for a control the directive is no longer attached to become inert.

Fixes angular#68744
@google-cla

This comment was marked as outdated.

Sign in to view
@pullapprove pullapprove Bot requested a review from atscott July 12, 2026 02:40
@ngbot ngbot Bot added this to the Backlog milestone Jul 12, 2026
@MedOussemaNjimi

MedOussemaNjimi commented Jul 12, 2026

Copy link
Copy Markdown
Author

@googlebot I signed it!

@JoostK

JoostK commented Jul 12, 2026

Copy link
Copy Markdown
Member

which assumes the value
accessor replaces the previously registered callback. Custom ControlValueAccessor
implementations commonly accumulate callbacks instead

I think the assumption is valid and should just be documented as interface contract. The provided CVA example has a memory leak by not unsubscribing, so that is a real problem that shouldn't be masked.

@MedOussemaNjimi

MedOussemaNjimi commented Jul 12, 2026

Copy link
Copy Markdown
Author

Fair point, thanks for the review! I agree the guard would mask the subscription leak in the accessor rather than surface it.

Happy to rework this PR. Which direction would you prefer?

  1. Documentation only — spell out on ControlValueAccessor.registerOnChange/registerOnTouched that each call must replace the previously registered callback (forms internals rely on this for cleanUpControl/rebinding).
  2. Documentation + a dev-mode warning — in addition to documenting the contract, emit an ngDevMode-only warning when a stale callback fires after the directive has been rebound to a different control. That would make the broken accessor (and its leak) visible to the developer instead of silently corrupting the previously bound control's value.

I'll update the PR accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@atscott atscott Awaiting requested review from atscott

Assignees

No one assigned

Labels

area: forms

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

Switching the [formControl] passed to a Control Value Accessor leaves the CVA partially attached to the old form control

3 participants

@MedOussemaNjimi @JoostK @izpapp