RFR: 8328977 : JEditorPane.setPage not thread-safe, pageLoader not cancelled

[Alexey Ivanov]

On Mon, 8 Apr 2024 06:09:16 GMT, Renjith Kannath Pariyangad <rkannathpari at openjdk.org> wrote:

> Hi Reviewers,
> 
> Added pageloader cancel before new page creation along with code restructuring. Moved all page loading calls inside synchronize to make it thread safe.
> 
> Regards,
> Renjith.

Changes requested by aivanov (Reviewer).

src/java.desktop/share/classes/javax/swing/JEditorPane.java line 485:

> 483:         Object postData = getPostData();
> 484:         if ((loaded == null) || !loaded.sameFile(page) || (postData != null)) {
> 485:             // different url or POST method, load the new content

Here's a mistake: if `postData != null`, we should proceed with the request. Thus, the condition in the `if`-block which verifies if the page needs reloading should also have this condition added.

src/java.desktop/share/classes/javax/swing/JEditorPane.java line 486:

> 484:             }
> 485:             return;
> 486:         }

This code should be moved above the previous `if`-block. Using `page.sameFile(loaded)` avoids the null-check.

Then if `page.getRef()` is `null`, I think we should still move the scrolling to start of the viewport as it's done in the `if`-block above.

src/java.desktop/share/classes/javax/swing/JEditorPane.java line 491:

> 489:         if ((postData == null) && (kit == null)) {
> 490:             return;
> 491:         }

This is another mistake, the editor kit changes when `getStream` method is called. Therefore, the condition to check `if (kit == null)` should be moved below line 513 where `getStream` is called.

src/java.desktop/share/classes/javax/swing/JEditorPane.java line 497:

> 495:             // we need to cancel background loading
> 496:             if (pageLoader != null) {
> 497:                 pageLoader.cancel(true);

Suggestion:

                pageLoader.cancel(true);
                pageLoader = null;

Let's assign `null` to indicate there's no active `pageLoader` any more.

src/java.desktop/share/classes/javax/swing/JEditorPane.java line 532:

> 530:         }
> 531:         read(in, doc);
> 532:         setDocument(doc);

Suggestion:

        }

        read(in, doc);
        setDocument(doc);

I recommend adding a blank line here to emphasise the separation: the body `if` never returns.

-------------

PR Review: https://git.openjdk.org/jdk/pull/18670#pullrequestreview-2009370111
PR Review Comment: https://git.openjdk.org/jdk/pull/18670#discussion_r1571061414
PR Review Comment: https://git.openjdk.org/jdk/pull/18670#discussion_r1571057721
PR Review Comment: https://git.openjdk.org/jdk/pull/18670#discussion_r1571064475
PR Review Comment: https://git.openjdk.org/jdk/pull/18670#discussion_r1571077133
PR Review Comment: https://git.openjdk.org/jdk/pull/18670#discussion_r1571075519