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

[Laurent Bourgès]

Jiri & Mario,

Here is an updated patch:
http://cr.openjdk.java.net/~lbourges/itw/patch_EDT.log.2

Mario, I tried webrev.ksh but it did not work with ITW repo. Do you have a
specific script for IcedTea ?
(I also uploaded the previous patch so you can see incremental diffs)

I fixed EDT reentrance in SwingUtils.invoke methods to fix the
CertificatePane issue.

See my comments below:

Le lun. 10 sept. 2018 à 13:03, Jiri Vanek <jvanek at redhat.com> a écrit :

> On 09/07/2018 04:50 PM, Laurent Bourgès wrote:
> > Jiri,
> >
> > Few comment below, but first PLEASE Go on integrating the current patch !
> >
> >      >      > +
> 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.
> >
> >     Then you can runit in --verbose, and you will see.
> >     is "nevermind" ok, or "well, if you insits"
> >     If it is so crucial, than it may be discussed to move it to default
> output.
> >
> >
> > I would vote to have LATER one good message indicating in the log (ALL)
> when the application starts:
> > 'Starting application [ + mainClass ]'
> > It will indicate that download phase ends and application startup phase
> starts.
>
> ok. Then please put it back. Can you put it to stderr? (using ERROR_ALL)
>

Fixed:
OutputController.getLogger().log(OutputController.Level.ERROR_ALL,
"Starting application [" + mainName + "] ...");


> >
> >
> >
> >     @@ -284,6 +292,11 @@
> >                    dialogTitle = "Security Warning";
> >                else if (dtype == DialogType.AUTHENTICATION)
> >                    dialogTitle = "Authentication Required";
> >     +        else if (dtype == DialogType.MISSING_ALACA) {
> >     +            dialogTitle = "Security Warning";
> >     +        } else if (dtype == DialogType.MATCHING_ALACA) {
> >     +            dialogTitle = "Security Warning";
> >     +        }
> >
> >     I overlooked this in first reading. How had you come over it? As it
> is only cosmetic change....
> >
> >
> > I prefer having these dialogs (with my Aspro2 app) showing a title.
> > Note: other titles are missing for few DialogType (later ?)
>
> Feel free to include them now or later
> Still, those would be nice to separate to another changeset (all the "new"
> titles.)
>

Removed.


>
> >
> >     Unless you have more opinion around " MESAGE_ALL?" part, or   you/me
> confirm  that
> >     splash/headless/sandbox  stopped working, I'm ok with this to be
> pushed to 1.7, with promises that
> >     push to future 1.8 will follow asap.
> >
> >     I will now start the integration suite of ITW (alas! have not made
> applets reasonably working, so
> >     javaws only), with hope to catch sandboxissue, if any. (will take
> long, as I need to create some
> >     base "before the patch")
> >     after that, I will do some manual testing
> >     after that I will push
> >     and then I will hope for 1.8 version, which I will test in same way
> as current with 1.7.
> >
> >     Does it sound good to you?
> >
> >
> > Yes, awesome.
> > How long does this testing phase take ? (1 day or 1 week ...)
>
> The suite runs abootu 6 hours. Without appelts, it is not so much covering.
> Anyway. Automated suite did not found any issue. My manual testing found
> one, so I'm afraid we will
> have one more round of review.
>
> Certificates details are dead, and missing
> http://icedtea.classpath.org/hg/icedtea-web/rev/4bac53123926 is not a
> cause (Although I'm going to
> backport it if 1.7.2 for EDT is going out).
> With your patch, the fialog never shows up, and itw (both javaws and itweb
> settings) get frozen.
> Without, it appears correctly.
> Most easy way to reproduce isa icedtea-web-image/bin/itweb-settings  ->
> certificates -> system ->
> any -> details - freeze (however the details are to be found when you are
> launching  any signed
> application so it is not an minor issue.)
>

I fixed the reentrance in SwingUtils.invokeAndWait() when called from EDT.
It was already in my (next) patch.

>
> Please note http://icedtea.classpath.org/hg/icedtea-web/rev/5290684409aa
> it will interact with the
> patch for bleeding edge.  Maybe it is worthy to backport it to 1.7 and to
> adapt your patch on top of it?
>

Please backport to 1.7.
I merged so the patch should not have these changes after backport:

@@ -741,8 +742,10 @@
             }
             if (!headless) {
                 try {
-                    new JWindow().getOwner();
-                } catch (Exception ex) {
+                    if (GraphicsEnvironment.isHeadless()) {
+                        throw new HeadlessException();
+                    }
+                } catch (HeadlessException ex) {


> >
> >
> >     I will start testing asap, and the minor update for "MESAGE_ALL?"
> can go in meantime
> >
> > Definitely, later.
>
> Well we are makeing a turn. Feel free to adapt it. I woudl vote to
> inclussion to stderr via
> ERROR_all, and I would vote to separate the titles hunk to separate
> changeset.
>

Agreed.


> >
> >
> >     Btw (realized now)  there is scary part which generate documentation
> for ITW, and
> >     icedtea-web.edt.debug should be mentioned. Howeve, I'm afraid there
> is no precedence for proeprty.
> >     I will look into it later.
> >
> >
> > I think this property is mainly for debugging purposes (internals); so I
> do not think it should be
> > advertised.
> ok.
>
> Sorry for troubles.
>   I was not investigating the freeze of certificates details, can you
> please look into it?
>

Done.

I tested again and both 'javaws -viewer' and itw-settings dialogs are OK.

Cheers,
Laurent
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20180910/39b212f7/attachment.html>