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

[Laurent Bourgès]

Mario,

> 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.
>

No problem, I didn't want to stress you.
Let's wait for Jiri to come back from holidays.


>
> 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.
>

Good idea, I will improve that.


> 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.
>

It is tricky here as most ViwableDialog methods are defered until show()
within the proper EDT (read the added comment at the beginning).

I can improve that to defer also the installPanel() method until show()
using a new method to queue the getPanel(this) ...
but it could have side-effect ? so I just wrap that call for now.


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

installPanel() or getPanel(this) must be called before adding the panel in
ViwableDialog => I will definitely rewrite that part.


>
> +        // 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?
>

Good idea, I also checked all ViwableDialog method usages and it is OK for
now.


>
>      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?
>

Yes, the idea is to discard redundant notifyObservers() calls due to
defered invokeLater() calls.


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

Ok, I propose to remove the 2 other classes and keep them on my side, if I
need to debug it again in the future.


>
> +    public static void checkEDT() {
>
> That doesn't have to be public.
>

Ok, but it helps when I want to insert checkEDT() in other ITW classes (as
I did during EDT debugging).


>
> 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"
>

It is not in my patch but in current code base.
Another example: ViwableDialog should be ViewableDialog ?


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

OK.


> 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).
>
Ok to remove @author


>
> 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).
>

OK, I will improve my patch:
- add SwingUtils.callOnAppContext()
- rewrite the installPanel() tricky part

Thanks for your review,
Laurent
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20180913/430814d5/attachment-0001.html>