HtmlUnit / htmlunit

HtmlUnit is a "GUI-Less browser for Java programs".
https://www.htmlunit.org
Apache License 2.0
863 stars 170 forks source link

Commit 4b96c6766c15ffc66ce399e33179b851049b5839 introduces regression #837

Open csware opened 1 month ago

csware commented 1 month ago

Commit 4b96c6766c15ffc66ce399e33179b851049b5839 causes Shibboleth login to fail (this is the first commit where this issue happens, identified using bisecting). Shibboleth receives a HTML post and then generates a HTML page with an onload action and/or a single submit button in a noscript block that redirects the request back to a calling web page.

Please revert that commit. Based on the commit description (and the separate commits that follow), I cannot see what problem it was trying to fix.

rbri commented 1 month ago

oh, this commit is part of the story to make the latest HTMX test suite working... will have a look.

csware commented 1 month ago

Reverting that commit (and adding final AbstractJavaScriptEngine<?> jsEngine = page.getWebClient().getJavaScriptEngine(); again) makes it work again.

diff --git a/src/main/java/org/htmlunit/html/DomElement.java b/src/main/java/org/htmlunit/html/DomElement.java
index 6469c1bd09..3cf4bce55d 100644
--- a/src/main/java/org/htmlunit/html/DomElement.java
+++ b/src/main/java/org/htmlunit/html/DomElement.java
@@ -1116,12 +1116,19 @@ public class DomElement extends DomNamespaceNode implements Element {
             stateUpdated = true;
         }

-        final ScriptResult scriptResult = doClickFireClickEvent(event);
-        final boolean eventIsAborted = event.isAborted(scriptResult);
+        final AbstractJavaScriptEngine<?> jsEngine = page.getWebClient().getJavaScriptEngine();
+        jsEngine.holdPosponedActions();
+        try {
+            final ScriptResult scriptResult = doClickFireClickEvent(event);
+            final boolean eventIsAborted = event.isAborted(scriptResult);

-        final boolean pageAlreadyChanged = contentPage != page.getEnclosingWindow().getEnclosedPage();
-        if (!pageAlreadyChanged && !stateUpdated && !eventIsAborted) {
-            changed = doClickStateUpdate(shiftKey, ctrlKey);
+            final boolean pageAlreadyChanged = contentPage != page.getEnclosingWindow().getEnclosedPage();
+            if (!pageAlreadyChanged && !stateUpdated && !eventIsAborted) {
+                changed = doClickStateUpdate(shiftKey, ctrlKey);
+            }
+        }
+        finally {
+            jsEngine.processPostponedActions();
         }

         if (changed) {
rbri commented 1 month ago

@csware do you have a test page where i can debug this?

csware commented 1 month ago

Unfortunately not, I collected the HTML pages of the Shibboleth login process, replaced all SAML data, but I cannot reproduce the issue with that page. There also must be something else.

Examples.zip Brokenpage.htm is the output of asXML() and Brokenpage2 is the HTML content collected using Firefox.

rbri commented 1 month ago

thanks, that helps a bit (and matches my theory) - will try to build a test case

rbri commented 1 week ago

Sorry for being not that responsive during the last days - was on a workshop and will be out for two more weeks to have a holiday break. Afterwards we can work together to get this finally done.