Skip to content

[java] ensure proper error message gets logged#12853

Merged
titusfortner merged 2 commits intotrunkfrom
error_msg
Oct 5, 2023
Merged

[java] ensure proper error message gets logged#12853
titusfortner merged 2 commits intotrunkfrom
error_msg

Conversation

@titusfortner
Copy link
Member

@titusfortner titusfortner commented Oct 2, 2023

Java Throwable class (at least for Java 11) does not set the detailMessage in the initCause method the same way it does in the way it does in the constructor:

    public Throwable(Throwable cause) {
        fillInStackTrace();
        detailMessage = (cause==null ? null : cause.toString());
        this.cause = cause;
    }

vs

    public synchronized Throwable initCause(Throwable cause) {
        if (this.cause != this)
            throw new IllegalStateException("Can't overwrite cause with " +
                                            Objects.toString(cause, "a null"), this);
        if (cause == this)
            throw new IllegalArgumentException("Self-causation not permitted", this);
        this.cause = cause;
        return this;
    }

@titusfortner titusfortner added the C-java Java Bindings label Oct 2, 2023
@titusfortner titusfortner requested a review from diemol October 2, 2023 19:15
diemol
diemol requested changes Oct 3, 2023
View reviewed changes
@pujagani
Copy link
Contributor

pujagani commented Oct 5, 2023

Good observation. This is a good start. I think similar changes might be required in other places as well. Example:

selenium/java/src/org/openqa/selenium/devtools/DevToolsException.java

Line 25 in c64ed7d

this(cause.getMessage(), cause);
,

selenium/java/src/org/openqa/selenium/bidi/BiDiException.java

Line 25 in c64ed7d

this(cause.getMessage(), cause);

@titusfortner
Copy link
Member Author

titusfortner commented Oct 5, 2023

@diemol I updated the code to make sure getCause() isn't null. It doesn't matter if getMessage() is null, it will just end up in the same place, but if getCause() were null for some reason, then... NPE.

@titusfortner titusfortner requested a review from diemol October 5, 2023 13:18
@titusfortner
Copy link
Member Author

titusfortner commented Oct 5, 2023

@pujagani I wanted to keep the changes limited to the one place I saw a specific problem (missing info in the logs I'm looking at). The code is still the same in Java 21, which leads me to believe it is intentional? I raised a ticket with Oracle to get their response.

We will review your report and have assigned it an internal review ID : 9076067. Depending upon the completeness of the report and our ability to reproduce the problem, either a new bug will be posted, or we will contact you for further information.

🤷

@titusfortner titusfortner merged commit 33c4122 into trunk Oct 5, 2023
@titusfortner titusfortner deleted the error_msg branch October 5, 2023 22:12
aguspe pushed a commit to aguspe/selenium that referenced this pull request Oct 22, 2023
@titusfortner @aguspe
[java] ensure proper error message gets logged (SeleniumHQ#12853)
4d42f90 
* [java] ensure proper error message gets logged

* check getCause for null instead of getMessage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@diemol diemol diemol approved these changes

Assignees

No one assigned

Labels

C-java Java Bindings

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@titusfortner @pujagani @diemol