SamProf / MatBlazor

Material Design components for Blazor and Razor Components
http://www.matblazor.com
MIT License
2.84k stars 386 forks source link

MatSelect Memory Leak #719

Open stoepa opened 4 years ago

stoepa commented 4 years ago

Describe the bug The bug that we have found is in terms of the MatSelect component, but I believe the MatCheckbox also suffers the same issue. If you have a MatSelect component in your page it renders the options in a seperate DOM element. I am talking about the UL element, the issue comes in that it never disposes,if you navigate away from that page and then navigate back to it, it recreates the UL element, thus doing navigation a couple of times would result in having n-number of UL elements with the inner LI or Span elements. I see from the repo that a dotnetobjectreference is created for each MatSelect element that is then stored in Javascript, thus leads to the memory reference never being disposed and can cause your application to freeze. The issue gets worse when you have a couple of MatSelects in a table and each row contains one MatSelect. If you click on any MatSelect it causes each one of them to actually rerender and create a additional UL element in the DOM.

To Reproduce Steps to reproduce the behavior:

  1. Create a new blazor-wasm project.
  2. Install the latest MatBlazor nuget package.
  3. Add a MatSelect to a page, also ensure you have atleast two pages, so that you can navigate.
  4. Run the Blazor app, click on the MatSelect and then close it, navigate to another page in the app and then navigate back to the first page.
  5. You will notice in developer tools that a new MSc-select UL is created in the Dom, keep on doing this and check the memory usage of the page climb by pressing ctrl+escape on the tab.
  6. If you want to see it get even worse add the MatSelect to a table that has multiple rows, with each row having a MatSelect and the data source of the MatSelect should preferably be a List and not static options.

Blazorfiddle link You can see this behavior on the MatBlazor website as well. Click on The MatSelect menu item, then navigate away and then navigate back.

Expected behavior The UL element in the DOM should get destroyed after using it.

Additional context The memory leaks happens especially if the MatSelect has a List data source and not a static set of options. The reason why I am pointing to a memory leaks is that the DOM elements keep replicating and they don't get destroyed. Also what is the most worrying is that for each MatSelect a dotnetobjectrererence is created and this resides on Javaadcripts side, and they don't get exposed. We can easily get the application to run up to 4gig of RAM thus crashing the browser.

I believe the issue to be that the UL element is getting renderes outside the tag and then Dotnet does not have a pointer to it, but this is just an assumption.

Please let me know if you need any additional information.

chris1411 commented 4 years ago

This seems like a huge issue that would prevent anyone from using the MatSelect in a production environment. Is there any update on when this may be addressed? I really like the MatBlazor controls and appreciate the efforts of the contributors, but it seems like alot of issues remain unresolved. It would be nice to see the existing components fixed and polished before moving on to add new components.

lindespang commented 4 years ago

@chris1411 Hi! I can only speak for myself but I haven't had so much time lately since I recently became a father which occupies a great deal of my time right now :)

But to address your last sentence - I can tell you that almost all work of the contributors goes to fixing bugs or reviewing PR's. I don't think anyone (or perhaps @SamProf ) of the active contributors is developing new components.

chris1411 commented 4 years ago

@lindespang I totatally understand and thanks for the clarification on development efforts.

After digging into this some more, it looks like if you use the FullWidth="true" on the MatSelect, it correctly anchors the list to the select element instead of the body, and therefore disposes of the list when the MatSelect is disposed. So there is a workaroud. However it looks like the MatMenu has the same issue since they are both built on top of the same mdc-menu components, and i don't believe there is a way to anchor that element currently.

I'm not very familiar with the mdc components, but is there any reason why we would not want to anchor the list to its parent element? Seems like it would fix this issue if we make that the default behavior for both the menu and the select.

lindespang commented 3 years ago

@SamProf Do you have an any answer to @chris1411 question? From what I can see in the changeset I can see that you added a condition to only anchor to parent if "Fullwidth" but otherwise anchor to body. (The proposed change from the PR was to always anchor to parent)

kristof12345 commented 3 years ago

Any update on this?

Simon-Miller commented 3 years ago

In my own commercial project using MatBlazor, I've noticed components not disposing. I created a base-class that uses BaseMatDomComponent as its base, so I can check the Disposed property before calling this.StateHasChanged(); That's enough to stop my own component from being disposed. However, if I inherit directly from Microsoft.AspNetCore.Components.ComponentBase, and add the Disposed property myself, then my components dispose as expected.

So this i still an issue. Although I must admit, I'm still stuck on 2.6.2 because a MatSelect doesn't work properly in server-side Blazor. So I'll apologise in advance if this has been resolved in never versions - - but the MatSelect still has not been solved.