Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Commit 4b96c6766c15ffc66ce399e33179b851049b5839 introduces regression #837

Closed
csware opened this issue Jul 31, 2024 · 12 comments
Closed

Commit 4b96c6766c15ffc66ce399e33179b851049b5839 introduces regression #837

csware opened this issue Jul 31, 2024 · 12 comments

Comments

@csware
Copy link
Contributor

csware commented Jul 31, 2024

Commit 4b96c67 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
Copy link
Member

rbri commented Jul 31, 2024

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

@csware
Copy link
Contributor Author

csware commented Jul 31, 2024

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
Copy link
Member

rbri commented Aug 1, 2024

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

@csware
Copy link
Contributor Author

csware commented Aug 2, 2024

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
Copy link
Member

rbri commented Aug 2, 2024

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

@rbri
Copy link
Member

rbri commented Sep 5, 2024

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.

@csware
Copy link
Contributor Author

csware commented Oct 29, 2024

I've a reproducer that I can provide confidentially.

@rbri
Copy link
Member

rbri commented Oct 29, 2024

Cool... please send a mail...

@csware
Copy link
Contributor Author

csware commented Oct 29, 2024

Sent.

@csware
Copy link
Contributor Author

csware commented Oct 29, 2024

It seems as if 4.5.0 fixed the issue, however, it behaves differently here compared to 4.3.0 as the "auto"-submit is not performed.

@rbri
Copy link
Member

rbri commented Oct 30, 2024

Have done a fix also. Will write some test for this and hopefully do a release at the beginning of next week.

rbri added a commit that referenced this issue Nov 2, 2024
@rbri
Copy link
Member

rbri commented Nov 2, 2024

@csware i think this is finally fixed. Have written i test case (based on your sample) that shows the problem is gone.

Hopefully i will do a release tomorrow.

@rbri rbri closed this as completed Nov 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants