Skip to content
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

Element: NRE by accessing RealParent #23320

Closed
MartyIX opened this issue Jun 27, 2024 · 3 comments · Fixed by #23405
Closed

Element: NRE by accessing RealParent #23320

MartyIX opened this issue Jun 27, 2024 · 3 comments · Fixed by #23405
Labels
area-controls-general General issues that span multiple controls, or common base classes such as View or Element fixed-in-8.0.70 fixed-in-9.0.0-preview.7.24407.4 p/1 Work that is important, and has been scheduled for release in this or an upcoming sprint platform/android 🤖 platform/iOS 🍎 platform/macOS 🍏 macOS / Mac Catalyst platform/tizen Samsung Tizen Devices (TV) platform/windows 🪟 s/triaged Issue has been reviewed s/verified Verified / Reproducible Issue ready for Engineering Triage t/bug Something isn't working
Milestone

Comments

@MartyIX
Copy link
Contributor

MartyIX commented Jun 27, 2024

Description

Running LayoutBenchmarker reports a NullReferenceException.

Steps to Reproduce

cd src/Core/tests/Benchmarks
dotnet build -c Release --framework net8.0 && dotnet run --framework net8.0 -c Release -- --runtimes net8.0 --filter *LayoutBenchmarker*

and I can observe in the output:

System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation.
 ---> System.NullReferenceException: Object reference not set to an instance of an object.
   at Microsoft.Maui.Controls.Element.SetParent(Element value) in Z:\maui\src\Controls\src\Core\Element\Element.cs:line 399
   at Microsoft.Maui.Controls.Element.OnChildAdded(Element child) in Z:\maui\src\Controls\src\Core\Element\Element.cs:line 578
   at Microsoft.Maui.Benchmarks.LayoutBenchmarker.GetLayoutHandlerIndex() in Z:\maui\src\Core\tests\Benchmarks\Benchmarks\LayoutBenchmarker.cs:line 26
   at BenchmarkDotNet.Autogenerated.Runnable_2.WorkloadActionNoUnroll(Int64 invokeCount) in Z:\maui\srtifacts\bin\Core.Benchmarks\Release\net8.0\44823321-481f-4fe8-b2f3-29b8a75b64f6\44823321-481f-4fe8-b2f3-29b8a75b64f6.notcs:line 695
   at BenchmarkDotNet.Engines.Engine.RunIteration(IterationData data)
   at BenchmarkDotNet.Engines.EngineStage.RunIteration(IterationMode mode, IterationStage stage, Int32 index, Int64 invokeCount, Int32 unrollFactor)
   at BenchmarkDotNet.Engines.EnginePilotStage.RunAuto()
   at BenchmarkDotNet.Engines.EnginePilotStage.Run()
   at BenchmarkDotNet.Engines.Engine.Run()
   at BenchmarkDotNet.Autogenerated.Runnable_2.Run(IHost host, String benchmarkName) in Z:\maui\srtifacts\bin\Core.Benchmarks\Release\net8.0\44823321-481f-4fe8-b2f3-29b8a75b64f6\44823321-481f-4fe8-b2f3-29b8a75b64f6.notcs:line 562
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeDirectByRefWithFewArgs(Object obj, Span`1 copyOfArgs, BindingFlags invokeAttr)
   --- End of inner exception stack trace ---
   at System.Reflection.MethodBaseInvoker.InvokeDirectByRefWithFewArgs(Object obj, Span`1 copyOfArgs, BindingFlags invokeAttr)
   at System.Reflection.MethodBaseInvoker.InvokeWithFewArgs(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters)
   at BenchmarkDotNet.Autogenerated.UniqueProgramName.AfterAssemblyLoadingAttached(String[] args) in Z:\maui\srtifacts\bin\Core.Benchmarks\Release\net8.0\44823321-481f-4fe8-b2f3-29b8a75b64f6\44823321-481f-4fe8-b2f3-29b8a75b64f6.notcs:line 57

The problem is here:

if (RealParent != null)
{
((IElementDefinition)RealParent).RemoveResourcesChangedListener(OnParentResourcesChanged);
if (value != null && (RealParent is Layout || RealParent is IControlTemplated))
Application.Current?.FindMauiContext()?.CreateLogger<Element>()?.LogWarning($"{this} is already a child of {RealParent}. Remove {this} from {RealParent} before adding to {value}.");
}

The issue seems to be that RealParent is de facto WeakReference<Element> and as such checking for null here is not sufficient to avoid an NRE.

Link to public reproduction project repository

No response

Version with bug

Nightly / CI build (Please specify exact version)

Is this a regression from previous behavior?

Not sure, did not test other versions

Last version that worked well

Unknown/Other

Affected platforms

Windows, I was not able test on other platforms

Affected platform versions

No response

Did you find any workaround?

I added var realParent = RealParent; and the benchmark finished OK.

Relevant log output

No response

@MartyIX MartyIX added the t/bug Something isn't working label Jun 27, 2024
@MartyIX
Copy link
Contributor Author

MartyIX commented Jun 27, 2024

@samhouts I believe that this affects all platforms. And if I read the code properly it can randomly crash for people under some scenarios.

@PureWeen PureWeen added this to the .NET 8 SR7 milestone Jun 27, 2024
@PureWeen PureWeen added the p/0 Work that we can't release without label Jun 27, 2024
@PureWeen
Copy link
Member

PureWeen commented Jun 27, 2024

AFAICT this isn't going to be something that happens in the wild much

The reason this happen in the benchmark is that the child views are all static

image

And the layout is purely scoped to the method body.
This means on subsequent runs these children are going to keep getting reparented and the previous parent is going to get GC'd

My gut tells me that recreating that set of conditions in the wild isn't going to happen very frequently.

We should still pattern match that if statement just to be safe.

@PureWeen PureWeen added p/1 Work that is important, and has been scheduled for release in this or an upcoming sprint area-controls-general General issues that span multiple controls, or common base classes such as View or Element and removed p/0 Work that we can't release without labels Jun 27, 2024
@PureWeen PureWeen modified the milestones: .NET 8 SR7, .NET 8 SR8 Jul 2, 2024
@ninachen03
Copy link

I can repro this issue on the Version 17.11.0 Preview 2.1

image

@ninachen03 ninachen03 added s/verified Verified / Reproducible Issue ready for Engineering Triage s/triaged Issue has been reviewed labels Jul 4, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in MAUI SDK Ongoing Jul 4, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Sep 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-general General issues that span multiple controls, or common base classes such as View or Element fixed-in-8.0.70 fixed-in-9.0.0-preview.7.24407.4 p/1 Work that is important, and has been scheduled for release in this or an upcoming sprint platform/android 🤖 platform/iOS 🍎 platform/macOS 🍏 macOS / Mac Catalyst platform/tizen Samsung Tizen Devices (TV) platform/windows 🪟 s/triaged Issue has been reviewed s/verified Verified / Reproducible Issue ready for Engineering Triage t/bug Something isn't working
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants