[rfc] [icedtea-web] Fix EDT hanging on OpenJDK 11

[Mario Torre]

On Thu, Sep 13, 2018 at 8:40 AM Laurent Bourgès
<bourges.laurent at gmail.com> wrote:
>
> Jiri,
>
> Could you push on 1.7 so I will prepare new patches during your holidays ?
>
> Mario, do you approve or not ?

Hi Laurent,

Just a little bit of patience, I asked for a second opinion, and I'm
not so familiar with the rest of the code base so I took some more
time to get around it.

Some nits and comments:

+            // Use SwingUtilities (not SwingUtils) to create EDT
within application context:
             SwingUtilities.invokeAndWait(new Runnable() {

Rather than adding the comment, it would be better to add this call to
SwingUtils, perhaps with a new method "callOnAppContext" or something.

I'm afraid a comment will be lost in time if there's more editing.

@@ -227,12 +227,18 @@
     private void initDialog() {
         String dialogTitle = createTitle();

+        // Note: ViwableDialog methods are defered until show():
         getViwableDialog().setTitle(dialogTitle);
         getViwableDialog().setModalityType(ModalityType.MODELESS);

         getViwableDialog().setDefaultCloseOperation(JDialog.DISPOSE_ON_CLOSE);

-        installPanel();
+        SwingUtils.invokeAndWait(new Runnable() {
+            @Override
+            public void run() {
+                installPanel();
+            }
+        });

Do I read it right, only part of this is set in the EDT? That doesn't
seem right to me, although there's probably no side effects in how you
did it.

Also, invokeAndWait are always a pain, they are primary target for
deadlocks, are you sure we don't want to "set and forget"
(invokeLater)?

+        // prune operations. May throw NPE if operations used after
createJDialog()
+        operations = null;

That sounds like a big warning to me, shouldn't be the method
documented the same way?

     private synchronized void updateModel(Boolean force) {
         observable.setChanged();
-        observable.notifyObservers(force);
+
+        SwingUtils.invokeLater(new Runnable() {
+
+            @Override
+            public void run() {
+                // avoid too much processing if already processed:
+                if (observable.hasChanged() || (Boolean.TRUE.equals(force))) {
+                    observable.notifyObservers(force);
+                }
+            }
+        });
     }

I'm assuming the observable is "setChanged" in a separate thread on
purpose and synchronisation with the notification isn't necessary?

Regarding the SwingUtils, I'm not happy about installing those repaint
managers, although I understand this is for debug only.

+    public static void checkEDT() {

That doesn't have to be public.

There's some minor mispelling here and in the comments, for example:

     public AdvancedProxySettingsDialog(DeploymentConfiguration config) {
         super((Frame) null, dialogTitle, true); // Don't need a parent.

It should probably be "Doesn't need a parent", or:

         if (!JNLPRuntime.isHeadless()) {
-            new JWindow().getOwner();
+            /* is it really usefull ? */
+            SwingUtils.getOrCreateWindowOwner();
         }

Useful with one "l". I'm also not sure if we have coding guidelines
here, the @author tag can be generally avoided, but I have no issues
if Jiri wants to keep it (the information about authors should be in
mercurial, not in the file itself).

I asked Martin to comment on the thread group stuff, although it looks
ok for me, I trust his opinion more.

After Martin gives you the OK, from my side it's ok to push if you
address the minor concern without a re-review (assuming you don't
drastically change things).

Cheers,
Mario
-- 
Mario Torre
Associate Manager, Software Engineering
Red Hat GmbH <https://www.redhat.com>
9704 A60C B4BE A8B8 0F30  9205 5D7E 4952 3F65 7898