Fix bug in TabPageCollection.RemoveAt (#7837)

[albahari]

Fixes #7837

Proposed changes

• Fix logic in TabPageCollection.RemoveAt to avoid removing incorrect TabPage when TabControl.Controls collection is out of sync with TabControls._tabPages. This can happen when Control.WmWindowPosChanged calls _parent.UpdateChildControlIndex, which calls Controls.SetChildIndex.

Customer Impact

• None

Regression?

• No

Risk

• Low

Test methodology

• Adhoc testing. There's just one of code, and the new implementation calls only public methods.

Microsoft Reviewers: Open in CodeFlow

[albahari]

• Do you think it's possible to come up with a regression test that would simulate the original issue, so that we can verify the fix?

It's tricky - the sequence of events to get the collections out of sync is complicated.

[RussKie]

How did you initially observe the issue? How can we be sure we've fixed it?

[ghost]

This submission has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 14 days.

It will be closed if no further activity occurs within 7 days of this comment.

[dreddy-work]

@albahari , Can you respond to the questions above? PRs with inactivity will be closed automatically after 14days.

[albahari]

Sorry for the delay - it took a while to get to a minimum repro. It's not until you subclass TabControl that the condition becomes apparent (or at least easy to reproduce). This discovery in itself has lead to finding a second bug. This second bug may in fact be a root cause (rendering the first bug fix redundant).

Control.UpdateChildControlIndex starts with the following code:

if (GetType().IsAssignableFrom(typeof(TabControl)))
{
   return;
}

This check is the wrong way around. It should be:

if (typeof(TabControl).IsAssignableFrom (GetType()))
{
   return;
}

Fixing this bug means that the tabControl.Controls collection (appears to) stay in sync with TabControls._tabPages. Assuming that the two collections are intended to always stay in sync (and there are no other conditions that could result in these collections being ordered differently), the first bug fix becomes redundant.

Here is a minimum repro. Note that it requires a running message loop, so TabPage can receive a WM_WINDOWPOSCHANGED message:

// Instantiate a subclassed TabControl with 5 pages:
var tc = new MyTabControl ();
var pages = Enumerable.Range (0, 5).Select (i => new TabPage ($"Page {i}")).ToArray();
tc.TabPages.AddRange (pages);	

// Select the last page:
tc.SelectedIndex = tc.TabPages.Count - 1;

// Show the TabControl on a form:
var form = new Form();
form.Controls.Add (tc);
form.Show();

// The change we made to SelectedIndex will result in a WM_WINDOWPOSCHANGED message which will 
// fire TabPage.WmWindowPosChanged, which will call TabControl.UpdateChildControlIndex, which will
// call Controls.SetChildIndex. This will reorder Controls without reordering TabPages.

// Notice that the TabPages and Controls collections are out of sync:
for (int i = 0; i < tc.TabCount; i++)
    Console.WriteLine ($"TabPage {tc.TabPages [i].Text} => {tc.Controls [i].Text}");
    
// I don't know whether having these collections out of sync in itself violates an invariant,
// or whether these collections are allowed to be differently-ordered. In any case, calling
// RemoveAt(int index) fails when the collections are not in sync:

// Remove the first page by position:
tc.TabPages.RemoveAt (0);

// We should be left with pages[1] and pages[2]
Debug.Assert (tc.TabPages[0] == pages [1]);      // Fails
Debug.Assert (tc.TabPages[1] == pages [2]);      // Fails

class MyTabControl : TabControl
{	
}

How should I proceed with this? Create a new PR for the second bug fix?

Note that while the original bug fix has virtually zero risk, the second bug fix could conceivably result in changed behavior in an (arguably obscure) scenario. Consider that someone instantiates the Control class directly: right now, the bug inappropriately short-circuits UpdateChildControlIndex when WM_WINDOWPOSCHANGED events are received. It might be prudent to mitigate that risk with this:

if (typeof(TabControl).IsAssignableFrom(GetType()) || GetType() == typeof(Control))
{
   return;
}

[dreddy-work]

Thank you @albahari for making time and sharing the findings here. We will review this and update you on next steps.

[dreddy-work]

@albahari , lets go ahead and update this PR with the new fix given current one is redundant after that. Also, make sure you add some unit tests / integration tests to cover the code path.

[albahari]

I've updated the PR and added an integration test that fails without the bugfix.

It doesn't make sense that the CI test would fail for X86 release on ActiveXFontMarshaler_RoundTripManagedFont() - perhaps this test needs to be repeated?

[lonitra]

Looks like there is some trailing whitespace that is causing build to complain. It is difficult for me to see where on web, but if you build on command line (cd to winforms repo and type 'build'), the same errors should show and help point you to where the trailing whitespace is.