[Bug]: NPE preventing HTML 5 dialogs from submitting

kwvanderlinde commented 1 year ago

Describe the Bug

In some circumstances, after reopening an HTML5 dialog, the dialog can no longer be closed via a submit button.

To Reproduce

Create a token "Lib:Dragon"
Add a macro to the token named "Action" with these contents:
```
[h: broadcast(macro.args)]
```

Add a second macro the token named "Open Dialog" with these contents:

[r, dialog5("Dialog", "input=1;closebutton=1"):{
<!DOCTYPE html>
<html lang="en">
<head></head>
<body>
    <form action="macro://Action@Lib:Dragon/none/Impersonated?" method="json">
    <button type="submit">Done</button>
</body>
</html>
}]

Click the "Open Dialog" macro to be presented the simple dialog.
Click the "Submit" button to close the dialog and see some output in the chat window from the "Action" macro.
Reopen the campaign in the same instance of MapTool (e.g. via File > Recent Campaigns).
Click the "Open Dialog" macro to open the dialog again
Click the "Submit" button in the dialog. Notice the dialog stays open and there is no output in chat.
Close the dialog via the "x" button.
Change the dialog name in the "Open Dialog" macro and open it again
Notice that the newly named dialog works as expected ... for now.

Expected Behaviour

The dialog form should be submitted, handled via the macro referenced in the form action, and the dialog should then be closed.

Screenshots

No response

MapTool Info

develop (f0c70b52a)

Desktop

Linux Mint 21.1

Additional Context

This is new on the develop branch and does not affect the 1.13 release.

Reopening the campaign is not strictly necessary, but I have found it tends to be more consistently reproducable that way.

The failure to submit is accompanied by this exception:

Exception in thread "JavaFX Application Thread" java.lang.NullPointerException
    at javafx.web/com.sun.webkit.WebPage.twkProcessMouseEvent(Native Method)
    at javafx.web/com.sun.webkit.WebPage.dispatchMouseEvent(WebPage.java:857)
    at javafx.web/javafx.scene.web.WebView.processMouseEvent(WebView.java:1098)
    at javafx.web/javafx.scene.web.WebView.lambda$registerEventHandlers$3(WebView.java:1224)
    at javafx.base/com.sun.javafx.event.CompositeEventHandler$NormalEventHandlerRecord.handleBubblingEvent(CompositeEventHandler.java:247)
    at javafx.base/com.sun.javafx.event.CompositeEventHandler.dispatchBubblingEvent(CompositeEventHandler.java:80)
    at javafx.base/com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:232)
    at javafx.base/com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:189)
    at javafx.base/com.sun.javafx.event.CompositeEventDispatcher.dispatchBubblingEvent(CompositeEventDispatcher.java:59)
    at javafx.base/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:58)
    at javafx.base/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
    at javafx.base/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
    at javafx.base/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
    at javafx.base/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
    at javafx.base/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
    at javafx.base/com.sun.javafx.event.EventUtil.fireEventImpl(EventUtil.java:74)
    at javafx.base/com.sun.javafx.event.EventUtil.fireEvent(EventUtil.java:54)
    at javafx.base/javafx.event.Event.fireEvent(Event.java:198)
    at javafx.graphics/javafx.scene.Scene$MouseHandler.process(Scene.java:3980)
    at javafx.graphics/javafx.scene.Scene.processMouseEvent(Scene.java:1890)
    at javafx.graphics/javafx.scene.Scene$ScenePeerListener.mouseEvent(Scene.java:2704)
    at javafx.graphics/com.sun.javafx.tk.quantum.EmbeddedScene.lambda$mouseEvent$4(EmbeddedScene.java:288)
    at java.base/java.security.AccessController.doPrivileged(AccessController.java:400)
    at javafx.graphics/com.sun.javafx.tk.quantum.EmbeddedScene.lambda$mouseEvent$5(EmbeddedScene.java:281)
    at javafx.graphics/com.sun.javafx.application.PlatformImpl.lambda$runLater$10(PlatformImpl.java:456)
    at java.base/java.security.AccessController.doPrivileged(AccessController.java:400)
    at javafx.graphics/com.sun.javafx.application.PlatformImpl.lambda$runLater$11(PlatformImpl.java:455)
    at javafx.graphics/com.sun.glass.ui.InvokeLaterDispatcher$Future.run(InvokeLaterDispatcher.java:95)
    at javafx.graphics/com.sun.glass.ui.gtk.GtkApplication._runLoop(Native Method)
    at javafx.graphics/com.sun.glass.ui.gtk.GtkApplication.lambda$runLoop$11(GtkApplication.java:316)
    at java.base/java.lang.Thread.run(Thread.java:1623)

kwvanderlinde commented 1 year ago

Bisected this to 1ddaa7b15

kwvanderlinde commented 1 year ago

This issue is not unique to submit buttons, . The same error happens for plain buttons and anchors (with or without an href), and probably plenty of other interactable elements (not <select> though...).

Unfortunately I haven't had much luck pinning down the root cause. The failure is in native code for some invisible resource, and so far everything I can see in Javaland looks fine. After reverting JavaFX to version 18, I could no longer reproduce the issue, while on JavaFX 19 and 20 it is pretty consistently showing up. So one solution would be to downgrade.

I tried out some different code changes as well to see if we can work around the problem and stay on JavaFX 20. One thing that worked was to discard the HTMLJFXPanel when closing the dialog, then create a fresh one when reopening the dialog again. Scroll reset was a casualty, but that could be fixed with a bit more effort. Have to try it yet, but I think the WebView is the key here, so we could keep the panel around but have the contained HTMLWebViewManager use a fresh WebView when closed/reopened. That approach would naturally preserve scroll reset.

Another thing I noticed was that the HTMLWebViewManager loads about:blank when closing the dialog. If I remove that, I can no longer recreate the issue. So this could be another option for addressing the issue (assuming we can find another way to accomplish its purpose). I don't really want to do that though, it mainly just indicates to me that we're probably not the ones at fault here, rather JavaFX is fumbling something.

I also added some threading checks to make sure all methods of HTMLWebViewManager are running where they should be, especially since there is some JavaFx / Swing interop there. There was a little bit of questionable behaviour, but even addressing that did nothing to avoid the current issue.

Azhrei commented 1 year ago

It’s a shame to hear it’s a regression in JFX 19 and 20 (or appears to be, at least). JFX 20 fixes an issue where the jQuery-UI library’s Tabs feature wasn’t working. (Same code work on v20 but not v18. I didn’t dig into why.)

kwvanderlinde commented 1 year ago

Progress update:

I found two different low-level aspect that seems to interact to create this issue:

As mentioned above, loading about:blank when closing the dialog.
Registering Java objects in the DOM to listen for submits.

Removing either will avoid the NPE from occuring (_of course, removing the latter also means submits don't work at all :sweatsmile:. But this is just debugging progress, not a suggestion for implementation).

I don't think we're doing anything wrong there. My working theory is that JavaFX is just losing / messing up the association between the Java listeners and their native peers when the page changes, possibly because it's cached the Java listener but doesn't invalidate the listener's peer when a new page is loaded :shrug: Whether or not this is accurate, the behaviour I'm seeing suggests we may be able to work around it by registering listener in JS rather than Java, and forward the events via our bridge code as that stuff seems to be working fine. Have yet to test this though.

cwisniew commented 1 year ago

Removing either will avoid the NPE from occuring (_of course, removing the latter also means submits don't work at all 😅. But this is just debugging progress, not a suggestion for implementation).

I have found another non useable work around... If I run it from the IDE in debug mode it never fails for me... I hate when that happens

kwvanderlinde commented 1 year ago

I've found that a fix went in for JavaFX 19 to allow garbage collection of EventTarget listeners: JDK-8088420. Quite possible that change included some undesirable behaviour.

I'm going to try reproduce this in a minimal example, and if I can I will report it to the JavaFX team. Then I'll keep looking for a workaround.

kwvanderlinde commented 1 year ago

I've found the key to be whether a GC cycle runs while the page is unloaded. So waiting a bit before reopening the dialog will more reliably result in a breakage. This also explains why reloading the campaign made it more consistent for me originally.

I've reproduced this in a minimal example that I will send to the JavaFX team. In the meantime, a very simple workaround has presented itself, so I'll put a PR together for that.

kwvanderlinde commented 1 year ago

Finally my bug report got added to the JDK bug tracker: JDK-8315915.

kwvanderlinde commented 1 year ago

We're seeing redundant events being fired since the attempted fix, e.g., when clicking macro links. Will have to look into how we can avoid this.

kwvanderlinde commented 1 year ago

Looks like #4285 has had other adverse effects. My mistaken assumption was that our MutationObserver would be called for all children added to the DOM, because subtree is set. This actually does not happen, meaning "deeply" nested nodes can be missed.

kwvanderlinde commented 1 year ago

Update on JDK-8315915: it looks like it will be fixed in JavaFX 22. Did some very quick testing and it seems to be the case.

kwvanderlinde commented 8 months ago

Closing this off as there was confirmation in Discord at the time that this was fixed, and there haven't been further reports to the contrary.

RPTools / maptool