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

[Mario Torre]

Hi Laurent,

I apologise for the delay, I have a couple more questions for you from
Martin and myself:

1. Why is EDT_DAEMON_THREAD_POOL needed?
2. In the previous code, there was a loop until
"!splashScreen.isSplashImageLoaded()", your code is functionally
different, other than just pushing things into the EDT, it also
refactor this call. I'm not very familiar with the inner intricacies
and I thought the previous call was redundant and only necessary due
to the cross thread synchronisation, but I rather ask your intentions
here that assume anything.
3. It seems that methods that use "observable" are synchronized.
However, the EDT will use "observable" and it's not synchronized. Is
this safe?

Cheers,
Mario
On Tue, Sep 25, 2018 at 9:15 PM Laurent Bourgès
<bourges.laurent at gmail.com> wrote:
>
> Any progress on this review ?
>
> Le jeu. 13 sept. 2018 à 15:55, Laurent Bourgès <bourges.laurent at gmail.com> a écrit :
>>
>> Mario,
>>
>> Here is the updated patch:
>> http://cr.openjdk.java.net/~lbourges/itw/patch_EDT.log.4
>>
>> Changes:
>> - added SwingUtils.callOnAppContext() used appropriately
>> - several SwingUtilities.invoke* replaced by SwingUtils.invoke*()
>> - removed 2 debugging EDT classes
>>
>> My answers, below:
>>
>>>
>>> > 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.
>>>
>>> Sure, let's revisit later then.
>>
>>
>> I tried but getPanel() calls SecurityDialog constructors that enqueue operations on ViwableDialog => chicken-egg problem.
>> => installPanel() MUST be called excatly at that place, and cannot be defered.
>>
>>>
>>> >> 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.
>>>
>>> Yes, thanks, users can do the same if they need to debug the EDT (as
>>> we can do too).
>>
>> Done.
>>>
>>>
>>> > It is not in my patch but in current code base.
>>> > Another example: ViwableDialog should be ViewableDialog ?
>>>
>>> Ops, you are right. If you don't mind fixing this line while we're
>>> there, otherwise just let it be and we'll go over another time.
>>
>> I left as is (as I forgot).
>>>
>>>
>>> > OK, I will improve my patch:
>>> > - add SwingUtils.callOnAppContext()
>>> > - rewrite the installPanel() tricky part
>>>
>>> Thanks!
>>>
>>> I don't need another review unless the patch changes substantially (or
>>> if you want me to go through your new changes for installPanel), if
>>> Martin approves you can push directly (or we'll wait for Jiri if you
>>> don't have push rights).
>>
>>
>> I modified few SwingUtilities usage by SwingUtils (tested OK), so you make a diff between .3 vs .4 (small) to have a quick look.
>>
>> As I do not have push rights, please push for me once the patch is approved.
>>
>> Cheers,
>> Laurent



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