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

[Laurent Bourgès]

Jiri,
Here is an updated patch, fixing all your concerns and I made more cleanups
and fixed code formatting.

Few answers, below:

>
> This is awesome work.
>

Thanks, it was quite tricky to fix EDT !


> One nitpick - autoformatting changes slipped in. About 200 lines. Do you
> mind to remove them from
> this patch?
>

Fixed.

> +
> OutputController.getLogger().log(OutputController.Level.MESSAGE_ALL,
> "Loading class: " + mainName);
> > +
>
> Is this really necessary as MESAGE_ALL?
>

Moved in logs, but I liked having it in my console as my app is starting
slowly (from home @ DSL) due to lots of http queries ... latency is then
important and I like knowing what's going on, nevermind.

>
> >               Class<?> mainClass =
> app.getClassLoader().loadClass(mainName);
> >
> >               Method main = mainClass.getMethod("main", new Class<?>[] {
> String[].class });
> > @@ -567,6 +569,7 @@
> >
> >               main.setAccessible(true);
> >
> > +
> OutputController.getLogger().log(OutputController.Level.MESSAGE_ALL,
> "Starting application ...");
> Same.
>

Fixed.

> diff -r bcbef8d7bbd6 netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java
> > --- a/netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java      Mon May 14
> 17:15:38 2018 +0200
> > +++ b/netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java      Wed Sep 05
> 22:49:43 2018 +0200
> > @@ -48,9 +48,9 @@
> >   import javax.net.ssl.SSLSocketFactory;
> >   import javax.net.ssl.TrustManager;
> >   import javax.swing.JOptionPane;
> > -import javax.swing.JWindow;
> >   import javax.swing.UIManager;
> >   import javax.swing.text.html.parser.ParserDelegator;
> > +import net.sourceforge.swing.SwingUtils;
> >
> >   import net.sourceforge.jnlp.DefaultLaunchHandler;
> >   import net.sourceforge.jnlp.GuiLaunchHandler;
> > @@ -740,15 +740,11 @@
> >                   headless = true;
> >               }
> >               if (!headless) {
> > -                try {
> > -                    new JWindow().getOwner();
> > -                } catch (Exception ex) {
> > +                if (SwingUtils.getOrCreateWindowOwner() == null) {
> >                       headless = true;
> > -                    OutputController.getLogger().log(ex);
> > -
> OutputController.getLogger().log(OutputController.Level.MESSAGE_ALL,
> Translator.R("HEADLESS_MISSCONFIGURED"));
> >                   }
> >               }
> > -        } catch (SecurityException ex) {
> > +        } catch (Exception e) {
>
> Is generic catch safe? getOrCreateWindowOwner do not add another exception.
>

Fixed.


> >           } finally {
> >               headlessChecked = true;
> >           }
> > diff -r bcbef8d7bbd6
> netx/net/sourceforge/jnlp/runtime/JNLPSecurityManager.java
> > --- a/netx/net/sourceforge/jnlp/runtime/JNLPSecurityManager.java
> Mon May 14 17:15:38 2018 +0200
> > +++ b/netx/net/sourceforge/jnlp/runtime/JNLPSecurityManager.java
> Wed Sep 05 22:49:43 2018 +0200
> > @@ -23,12 +23,12 @@
> >   import java.security.AccessControlException;
> >   import java.security.Permission;
> >
> > -import javax.swing.JWindow;
> >
> >   import net.sourceforge.jnlp.security.SecurityDialogs.AccessType;
> >   import net.sourceforge.jnlp.services.ServiceUtil;
> >   import net.sourceforge.jnlp.util.logging.OutputController;
> >   import net.sourceforge.jnlp.util.WeakList;
> > +import net.sourceforge.swing.SwingUtils;
> >   import sun.awt.AWTSecurityManager;
> >   import sun.awt.AppContext;
> >
> > @@ -114,7 +114,11 @@
> >           // called for it (and not disposed).
> >
> >           if (!JNLPRuntime.isHeadless()) {
> > -            new JWindow().getOwner();
> > +            /*
> > +            should be shared with JNLPRuntime as checkHeadless() does
> the same
> > +            or totally useless ?
> > +            */
> > +            SwingUtils.getOrCreateWindowOwner();
>
> Fair comment, and tbh, I dont know.
> >           }
> >
> >           mainAppContext = AppContext.getAppContext();
> > diff -r bcbef8d7bbd6
> netx/net/sourceforge/jnlp/security/SecurityDialog.java
> > --- a/netx/net/sourceforge/jnlp/security/SecurityDialog.java  Mon May 14
> 17:15:38 2018 +0200
> > +++ b/netx/net/sourceforge/jnlp/security/SecurityDialog.java  Wed Sep 05
> 22:49:43 2018 +0200
> Many many formatincg changes. If you wish, I can push the formatting of
> this class ahead of time.
>

I reverted formatting changes.


>
> > diff -r bcbef8d7bbd6
> netx/net/sourceforge/jnlp/security/SecurityDialogMessageHandler.java
> > ---
> a/netx/net/sourceforge/jnlp/security/SecurityDialogMessageHandler.java
> Mon May 14 17:15:38 2018 +0200
> > +++
> b/netx/net/sourceforge/jnlp/security/SecurityDialogMessageHandler.java
> Wed Sep 05 22:49:43 2018 +0200
> > @@ -247,7 +247,7 @@
> >               msg.lock.release();
> >           }
> >       }
> > -        /**
> > +    /**
> Blame on ITW, but stil the same.
>

Fixed.


> >        * Post a message to the security event queue. This message will
> be picked
> >        * up by the security thread and used to show the appropriate
> security
> >        * dialog.
> ...
>  > -        getViwableDialog().addWindowListener(adapter);
>  > +//        getViwableDialog().addWindowListener(adapter);
> Why this change?
>

I reverted that but I left my explanation in the code saying that is is
dead code that never happens as JDialog != SecurityDialog ... so this
listener is useless.



> Can you please use license from icedtea?


Fixed in the 3 new files.

> +public final class SwingUtils {
> > +
> > +    private static final boolean INFO_DIALOG = false;
> > +    private static final boolean DEBUG_EDT = false;
> > +    private static final boolean TRACE_EDT = false;
>
> Does any one f those sense to been enabled by ITW verbose mode? Does
> anyone have sense to be enabled
> via -Djava.awt.itw.whatever ?
>

Only DEBUG_EDT may be helpful, so I fixed:
DEBUG_EDT = System.getProperty("icedtea-web.edt.debug",
"false").equalsIgnoreCase("true");


>
>
> You are using sout here. This is violating ITW logging bottle neck.
> However ITW have logging -
> unless disabled in itw settngs - bound to console, which is awt. So it can
> be contra productive.
>

I wrapped such calls in trace(String) and explained that it is only for
TRACING/DEBUG EDT (ITW internals) so logging / console may interfere as it
is using also EDT (chicken-egg problem) !


> I do not have preference for those two classes. I have never been so deep
> in AWT debugging to need
> this, But I'm happy to learn the approach.  And tehy can go in.
>

Cheers,
Laurent
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20180906/276ab42d/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch_EDT.log
Type: text/x-log
Size: 51864 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20180906/276ab42d/patch_EDT-0001.log>