Skip to content

Fix IndexOutOfRangeException when an added/removed attribute spans a buffer size boundary #50110

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 22, 2023

Conversation

SteveSandersonMS
Copy link
Member

@SteveSandersonMS SteveSandersonMS commented Aug 16, 2023

NOTE: I'm not actually proposing this for servicing right now, because it doesn't look like there would be a major benefit from including it in RC1, and including it in RC2 would be perfectly sufficient. However I'm writing up the ask mode template anyway just in case people disagree.

Extremely rarely, component rendering could cause IndexOutOfRangeException.

Description

If a component rendered a very specific number of frames, and then updated to render exactly one more frame, and that new frame was an attribute, and it was the last frame in the component, the diffing system would throw IndexOutOfRangeException. This was due to a bug in checking whether there are more attributes to process.

Fixes #49192

Customer Impact

Fairly low, because it's really hard to reproduce this, even if you're doing so on purpose. However, total correctness of the Blazor renderer is a high priority for us, and we do not want any edge cases where it can fail.

In the event that the issue does occur, it would behave like an unhandled exception from application code:

  • For Blazor WebAssembly, the yellow error bar would appear, but the app would continue running
  • For Blazor Server, the current user's circuit would be terminated, but other server activity would be unaffected
  • For Blazor WebView, the unhandled exception callback would run

Regression?

  • Yes
  • No

[If yes, specify the version the behavior has regressed from]

Risk

  • High
  • Medium
  • Low

Low because the code change is small enough (4 lines) to reason clearly that it only makes a difference in the case where it would have failed before. The updated boundary condition is simple to understand.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

@SteveSandersonMS SteveSandersonMS added the ask-mode This issue / PR is a patch candidate which we will bar-check internally before patching it. label Aug 16, 2023
@SteveSandersonMS SteveSandersonMS requested a review from a team as a code owner August 16, 2023 09:33
@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Aug 16, 2023
@SteveSandersonMS SteveSandersonMS changed the title Fix OutOfRangeException when an added/removed attribute spans a buffer size boundary Fix IndexOutOfRangeException when an added/removed attribute spans a buffer size boundary Aug 16, 2023
@SteveSandersonMS SteveSandersonMS added the Servicing-consider Shiproom approval is required for the issue label Aug 16, 2023
Assert.Equal("myattribute_final", entry.RemovedAttributeName);
});
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both of these new unit tests do fail with IndexOutOfRangeException without the fix, and of course pass with the fix.

@SteveSandersonMS SteveSandersonMS removed Servicing-consider Shiproom approval is required for the issue ask-mode This issue / PR is a patch candidate which we will bar-check internally before patching it. labels Aug 16, 2023
@SteveSandersonMS SteveSandersonMS added this to the 8.0-rc2 milestone Aug 16, 2023
@SteveSandersonMS SteveSandersonMS self-assigned this Aug 16, 2023
@SteveSandersonMS SteveSandersonMS changed the base branch from release/8.0-rc1 to release/8.0 August 16, 2023 10:04
@dotnet dotnet deleted a comment Aug 16, 2023
SteveSandersonMS and others added 2 commits August 16, 2023 14:09
Co-authored-by: campersau <buchholz.bastian@googlemail.com>
There's nothing in the syntax to indicate whether NoSuchAttributeFrame would be copied or not
@mkArtakMSFT mkArtakMSFT merged commit 78708d6 into release/8.0 Aug 22, 2023
@mkArtakMSFT mkArtakMSFT deleted the stevesa/fix-attributes-at-buffer-boundaries branch August 22, 2023 18:47
@ghost ghost modified the milestone: 8.0-rc2 Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants