Calling VaadinService.access or accessSynchronously leaks UI ThreadLocals

[pleku]

@tmattsso commented on Wed Aug 28 2019

When the method is called, CurrentInstance will store a copy of all current instances, then override two objects; VaadinService and VaadinSession. If there is a UI object in the map, it is not cleared (current request and response might be affected too). This means that clients can call UI.getCurrent inside the access block and get a random UI instance.

Either the UI mapping should explicitly be set to null, or the map be cleared, before running client code. The latter would be better, because it will also clear (temporarily) any other mappings that a user might have done for another session.

[archiecobbs]

Proposed patch:

diff --git a/flow-server/src/main/java/com/vaadin/flow/server/VaadinService.java b/flow-server/src/main/java/com/vaadin/flow/server/VaadinService.java
index 237fae7553..80e40b7fe6 100644
--- a/flow-server/src/main/java/com/vaadin/flow/server/VaadinService.java
+++ b/flow-server/src/main/java/com/vaadin/flow/server/VaadinService.java
@@ -2050,12 +2050,13 @@ public abstract class VaadinService implements Serializable {
 
         FutureAccess pendingAccess;
 
         // Dump all current instances, not only the ones dumped by setCurrent
         Map<Class<?>, CurrentInstance> oldInstances = CurrentInstance
                 .getInstances();
+        CurrentInstance.clearAll();
         CurrentInstance.setCurrent(session);
         try {
             while ((pendingAccess = session.getPendingAccessQueue()
                     .poll()) != null) {
                 if (!pendingAccess.isCancelled()) {
                     pendingAccess.run();
        while ((pendingAccess = session.getPendingAccessQueue()

[archiecobbs]

This bug is over five years old and still happening.

Is there a problem with fixing this?? If so what is the problem??

Just to be clear, one of the many side effects of this bug is that UI.getCurrent() returns the wrong value.

Moreover, this is trivial to fix!

Reproducer:

• Build and run https://github.com/archiecobbs/vaadin-grid-load-fail-issue
• Connect to http://localhost:8080/example/example/other in two different browser tabs
• Click the button in the first root browser tab
• Click the button in the non-first root browser tab
• Notice how UI.getCurrent() in the non-first root tab reports the wrong UI

Video of the reproducer:

VaadinUIBug.mov

Here's a more correct patch:

--- a/flow-server/src/main/java/com/vaadin/flow/server/VaadinService.java
+++ b/flow-server/src/main/java/com/vaadin/flow/server/VaadinService.java
@@ -2125,11 +2125,12 @@ public abstract class VaadinService implements Serializable {
         // Dump all current instances, not only the ones dumped by setCurrent
         Map<Class<?>, CurrentInstance> oldInstances = CurrentInstance
                 .getInstances();
-        CurrentInstance.setCurrent(session);
         try {
             while ((pendingAccess = session.getPendingAccessQueue()
                     .poll()) != null) {
                 if (!pendingAccess.isCancelled()) {
+                    CurrentInstance.clearAll();
+                    CurrentInstance.setCurrent(session);
                     pendingAccess.run();
 
                     try {

[Legioth]

Thanks for the reminder and for the PR!

Is there a problem with fixing this?? If so what is the problem??

I suspect it just wasn't critical enough when it was filed and then it feel through the cracks.
I don't see any problem other than a very small risk that existing code out there accidentally depends on the bug but I don't think that's reason enough to leave it broken.

[archiecobbs]

I don't see any problem other than a very small risk that existing code out there accidentally depends on the bug but I don't think that's reason enough to leave it broken.

OK great.

IMHO the biggest reason to apply this is that it introduces "randomness" (using air quotes because in some applications it never causes a problem, in some applications it always causes a problem, and in some application it's truly random) that makes other bugs much harder to reproduce and track down, like #20240 (which took me a over week to figure out).

Please let me know if you need anything else from me to get this pushed through. Thanks.

[vaadin-bot]

This ticket/PR has been released with Vaadin 14.12.2.

[vaadin-bot]

This ticket/PR has been released with Vaadin 24.3.19.

[vaadin-bot]

This ticket/PR has been released with Vaadin 24.4.14.

[vaadin-bot]

This ticket/PR has been released with Vaadin 23.5.8.

[vaadin-bot]

This ticket/PR has been released with Vaadin 24.5.1.

[vaadin-bot]

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

[jameskerrtaskize]

@Legioth @archiecobbs
Do you know if its valid for us to call VaadinServletRequest.getCurrent() in a beforeEnter method? It always used to return a value but since this change, it may now return null.

I can go into more detail but if you tell me that we should never call VaadinServletRequest.getCurrent() then we'll have to go and rethink.

[archiecobbs]

Do you know if its valid for us to call VaadinServletRequest.getCurrent() in a beforeEnter() method?

I don't think it's safe to assume, in general, that there is a "current request" when beforeEnter() is invoked.

For example, a user may be looking at a screen that says "Operation in progress..." and waiting for some background task to complete.

When that task finally does complete, it may then as a result install a new view to display the results of the operation.

So while it will need to know a UI in order to apply the new view to it, there won't be any HTTP request associated with this action, since it originated from a background thread.

[jameskerrtaskize]

Thanks for the quick response. We had mistakenly thought that beforeEnter would run on a request thread. I can now see that it may run on any thread and it makes sense that VaadinServletRequest.getCurrent() should return null in that instance.

[jameskerrtaskize]

Not that it matters but if you call access within another access call, the task also gets put into the queue and executed later. So it may still be the same [request] thread running the task but the request is null. It doesn't matter now that we know its not a valid thing to do anyway.