fix: collect changes until UI is not dirty (#15517) (#15521)

When UidlWriter collects changes to send to the client, it may happen
that UI is still dirty because features may enqueue additional changes.
As a side effect, if UI is still dirty after UIDL response is written,
a useless PUSH action is performed.
This change performs changes collection until the UI is no more dirty.

Fixes #14887

flow-server/src/main/java/com/vaadin/flow/server/communication/UidlWriter.java

@@ -30,6 +30,7 @@
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
+import java.util.function.Consumer;
 import java.util.stream.Stream;
 
 import org.apache.commons.io.IOUtils;
@@ -412,7 +413,7 @@ private void encodeChanges(UI ui, JsonArray stateChanges) {
         stateTree.runExecutionsBeforeClientResponse();
 
         Set<Class<? extends Component>> componentsWithDependencies = new LinkedHashSet<>();
-        stateTree.collectChanges(change -> {
+        Consumer<NodeChange> changesCollector = change -> {
             if (attachesComponent(change)) {
                 ComponentMapping.getComponent(change.getNode())
                         .ifPresent(component -> addComponentHierarchy(ui,
@@ -422,7 +423,21 @@ private void encodeChanges(UI ui, JsonArray stateChanges) {
             // Encode the actual change
             stateChanges.set(stateChanges.length(),
                     change.toJson(uiInternals.getConstantPool()));
-        });
+        };
+        // A collectChanges round may add additional changes that needs to be
+        // collected.
+        // For example NodeList.generateChangesFromEmpty adds a ListClearChange
+        // in case of remove has been invoked previously
+        // Usually, at most 2 rounds should be necessary, so stop checking after
+        // five attempts to avoid infinite loops in case of bugs.
+        int attempts = 5;
+        while (stateTree.hasDirtyNodes() && attempts-- > 0) {
+            stateTree.collectChanges(changesCollector);
+        }
+        if (stateTree.hasDirtyNodes()) {
+            getLogger().warn("UI still dirty after collecting changes, "
+                    + "this should not happen and may cause unexpected PUSH invocation.");
+        }
 
         componentsWithDependencies
                 .forEach(uiInternals::addComponentDependencies);

flow-server/src/test/java/com/vaadin/flow/server/communication/UidlWriterTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2000-2020 Vaadin Ltd.
+ * Copyright 2000-2022 Vaadin Ltd.
  *
  * Licensed under the Apache License, Version 2.0 (the "License"); you may not
  * use this file except in compliance with the License. You may obtain a copy of
@@ -31,16 +31,19 @@
 import org.junit.Test;
 
 import com.vaadin.flow.component.Component;
+import com.vaadin.flow.component.HasComponents;
 import com.vaadin.flow.component.Tag;
 import com.vaadin.flow.component.UI;
 import com.vaadin.flow.component.dependency.HtmlImport;
 import com.vaadin.flow.component.dependency.JavaScript;
 import com.vaadin.flow.component.dependency.StyleSheet;
 import com.vaadin.flow.component.internal.PendingJavaScriptInvocation;
+import com.vaadin.flow.component.internal.UIInternals;
 import com.vaadin.flow.component.internal.UIInternals.JavaScriptInvocation;
 import com.vaadin.flow.dom.Element;
 import com.vaadin.flow.dom.ElementFactory;
 import com.vaadin.flow.internal.JsonUtils;
+import com.vaadin.flow.internal.StateTree;
 import com.vaadin.flow.router.ParentLayout;
 import com.vaadin.flow.router.Route;
 import com.vaadin.flow.router.RouteConfiguration;
@@ -56,14 +59,15 @@
 import elemental.json.JsonArray;
 import elemental.json.JsonObject;
 import elemental.json.JsonValue;
+import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.core.Is.is;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.when;
 
 @NotThreadSafe
@@ -173,6 +177,12 @@ public static class SuperParentClass extends Component
             implements RouterLayout {
     }
 
+    @Tag("components-container")
+    public static class ComponentsContainer extends Component
+            implements HasComponents {
+
+    }
+
     @After
     public void tearDown() {
         if (mocks != null) {
@@ -371,6 +381,43 @@ public void resynchronizationRequested_responseFieldContainsResynchronize()
                 response.getBoolean(ApplicationConstants.RESYNCHRONIZE_ID));
     }
 
+    @Test
+    public void createUidl_allChangesCollected_uiIsNotDirty() throws Exception {
+        UI ui = initializeUIForDependenciesTest(new TestUI());
+
+        ComponentsContainer container = new ComponentsContainer();
+        container.add(new ChildComponent());
+        ui.add(container);
+        // removing all elements causes an additional ListClearChange to be
+        // added during collectChanges process
+        container.removeAll();
+
+        UidlWriter uidlWriter = new UidlWriter();
+        uidlWriter.createUidl(ui, false, true);
+
+        assertFalse("UI is still dirty after creating UIDL",
+                ui.getInternals().isDirty());
+    }
+
+    @Test
+    public void createUidl_collectChangesUIStillDirty_shouldNotLoopEndlessly()
+            throws Exception {
+        UI ui = initializeUIForDependenciesTest(spy(new TestUI()));
+        StateTree stateTree = spy(ui.getInternals().getStateTree());
+        UIInternals internals = spy(ui.getInternals());
+
+        when(ui.getInternals()).thenReturn(internals);
+        when(internals.getStateTree()).thenReturn(stateTree);
+        when(stateTree.hasDirtyNodes()).thenReturn(true);
+
+        UidlWriter uidlWriter = new UidlWriter();
+        uidlWriter.createUidl(ui, false, true);
+
+        assertTrue(
+                "Simulating collectChanges bug and expecting UI to be still dirty after creating UIDL",
+                ui.getInternals().isDirty());
+    }
+
     private void assertInlineDependencies(List<JsonObject> inlineDependencies,
             String expectedPrefix) {
         assertThat("Should have an inline dependency", inlineDependencies,