Memory leak when repeatedly setting DataProvider

[edler-san]

Minimum example:

@Route("")
public class MainView extends VerticalLayout {

public MainView() {
    Grid<Object> grid = new Grid<>();
    ListDataProvider<Object> provider = new ListDataProvider<>(new ArrayList<>());
    grid.setDataProvider(provider);

    add(grid);

    Button refreshButton = new Button("Refresh");
    refreshButton.addClickListener(e -> provider.refreshAll());
    add(refreshButton);
    }
}

When the setDataProvider method is called, it internally will call "handleAttach" on the DataCommunicator. This will register a DataProviderListener and return a DataProviderUpdateRegistration. However, when adding the Grid to a layout, handleAttach will be called again which will register another DataProviderListener, because the handleAttach method of the DataCommunicator does not check if the DataProviderUpdateRegistration already has a value.

Now there are two identical DataProviderListeners, with one having no registration to remove it. Once you update/refresh the grid, BOTH of these Listeners will be called in the fireEvent method of the AbstractDataProvider, which causes unnecessary calls to the reset() method. When you then click the Refresh button below, you will see that the fireEvent method will have three listeners, where two of those will point to the same DataCommunicator, and so on.

Tested with Vaadin 14.6.6.

[TatuLund]

Note, this can happen with ComboBox too.

[mshabarov]

Reopened, because the proposed change breaks ComboBox filtering tests and lead to regression. Need to consider a fix on ComboBox side.

[mshabarov]

The fix on ComboBox side is proposed in the corresponding PR.

Here are some clarifications why the tests in CB failed:

How it worked before both fixes (in Flow and ComboBox):

1. DataCommunicator adds a data provider listener upon its setDataProvider method.
2. ComboBox adds a data provider listener upon its setDataProvider method.
3. DataCommunicator adds one more data provider listener upon attach event.
4. ComboBox adds one more data provider listener upon attach event.

So finally DC listener comes before CB listener and this works fine.

How it worked after the fix in Flow:

1. DataCommunicator adds a data provider listener upon its setDataProvider method.
2. ComboBox adds a data provider listener upon its setDataProvider method.
3. DataCommunicator REMOVES the old data provider listener upon attach event and adds a new one.
4. ComboBox adds one more data provider listener upon attach event.

So now the CB listener comes first (because the first listener in the queue has been removed), and this violates the order of listener execution. And this causes a side-effects and test failures.

How it works after the fix in Flow and ComboBox:

1. DataCommunicator adds a data provider listener upon its setDataProvider method.
2. ComboBox adds a data provider listener upon its setDataProvider method.
3. DataCommunicator REMOVES the old data provider listener upon attach event and adds a new one.
4. ComboBox REMOVES the old data provider listener upon attach event and adds a new one.

Thus, there are only two listeners in the end and they are in the right order -> works fine.

[vaadin-bot]

This ticket/PR has been released with platform 21.0.0.rc1 and is also targeting the upcoming stable 21.0.0 version.

[KiteDX]

What about the Vaadin14 version this bug was initially reported for? I can't find any mention of that version in here.

Ah sorry... missed the "10+" part... I only looked at the labels.

[mshabarov]

@KiteDX , the fix for Vaadin 14 is pending Flow 2.6.8 release, then it will be included into next Vaadin 14.6.X version.

[vaadin-bot]

This ticket/PR has been released with platform 20.0.7.

[vaadin-bot]

This ticket/PR has been released with platform 22.0.0.alpha2 and is also targeting the upcoming stable 22.0.0 version.