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

[Jiri Vanek]

Hello!

The patch is pushed to both head and 1.7

During the applying, I noticed following:
ThreadCheckingRepaintManager was returned, however is unused.  Is that intentional?
Together with it, acordingly, +    private static final boolean DEBUG_EDT = 
System.getProperty("icedtea-web.edt.debug", "false").equalsIgnoreCase("true"); was removed.

I think one without another do not have sense. I would vote for make it enable-able. If not then it 
should be removed, as it is now dead code.

Is the next step to rmeove ThreadCheckingRepaintManager or  to make it anebale-able?

Tahnx a lot for contribution!
   J.

On 10/1/18 4:05 PM, Mario Torre wrote:
> Thank you for contributing!
> 
> Cheers,
> Mario
> On Mon, Oct 1, 2018 at 3:48 PM Laurent Bourgès
> <bourges.laurent at gmail.com> wrote:
>>
>> Thank you for reviews,
>> Laurent
>>
>> Le lun. 1 oct. 2018 à 15:16, Mario Torre <neugens at redhat.com> a écrit :
>>>
>>> Hi Laurent,
>>>
>>> Yes, I approve.
>>>
>>> I think we need to rework the threading aspect though.
>>>
>>> Jiri, could you please push the patch for Laurent?
>>>
>>> Cheers,
>>> Mario
>>>
>>> On Mon, Oct 1, 2018 at 12:13 PM Laurent Bourgès
>>> <bourges.laurent at gmail.com> wrote:
>>>>
>>>> Mario,
>>>> Do you approve the latest webrev, as it is ?
>>>> Let's move forward, after this patch integrated.
>>>> Laurent
>>>>
>>>> Le lun. 1 oct. 2018 à 11:17, Jiri Vanek <jvanek at redhat.com> a écrit :
>>>>>
>>>>> ...
>>>>>>
>>>>>>       >     3. It seems that methods that use "observable" are synchronized.
>>>>>>       >     However, the EDT will use "observable" and it's not synchronized. Is
>>>>>>       >     this safe?
>>>>>>       >
>>>>>>       >
>>>>>>       > Ok, fixed:
>>>>>>       >                 synchronized(observable) {
>>>>>>       >                     if (observable.hasChanged() ||
>>>>>>       > (Boolean.TRUE.equals(force))) {
>>>>>>       >                         observable.notifyObservers(force);
>>>>>>       >                     }
>>>>>>       >                 }
>>>>>>
>>>>>>      Right, but I don't think this addresses the issue, one is synchronized
>>>>>>      on the observable, the other methods are on the JavaConsole, so we
>>>>>>      should probably pick one.
>>>>>>
>>>>>>
>>>>>> Sorry, I did not catch the problem.
>>>>>> My fix is very minimal and this patch does not fix synchronization issues already present in ITW.
>>>>>> I tested javaws with the console opened and did not notice any problem.
>>>>>>
>>>>>>
>>>>>>      By going through the code though, it seems like we could just ensure
>>>>>>      that updateModel() always run in the EDT and remove all other
>>>>>>      synchronised keywords from the methods, the only user of this method
>>>>>>      which can execute outside the EDT is addMessage, but otherwise all other
>>>>>>      consumers are executing in the EDT.
>>>>>>
>>>>>>
>>>>>> Could we postpone this point later as I did not want to rewrite too much the JavaConsole ?
>>>>>> You propose to remove all 'synchronized' keywords in JavaConsole, I am pretty sure the
>>>>>
>>>>> I would strongly advice against removing the synchronizations. The logging bottleneck is lazily
>>>>> initiated singleton, and there were many troubles without it. Maybe with plugin gone, the troubles
>>>>> will decrease a lot, But i doubt.
>>>>>
>>>>> On contrary:
>>>>>
>>>>>   >on the observable, the other methods are on the JavaConsole, so we
>>>>>   >     should probably pick one.
>>>>>
>>>>> Sounds like straightforward fix. Anyway, this is on your judgement.
>>>>>
>>>>>> synchronization overhead does not hurt here...
>>>>>> If you could propose an alternative, I am OK to integrate it.
>>>>>
>>>>> Please, as another changeset (if any).
>>>>>
>>>>>
>>>>>
>>>>> Thanx!
>>>>>     J.
>>>
>>>
>>>
>>> --
>>> Mario Torre
>>> Associate Manager, Software Engineering
>>> Red Hat GmbH <https://www.redhat.com>
>>> 9704 A60C B4BE A8B8 0F30  9205 5D7E 4952 3F65 7898
> 
> 
> 


-- 
Jiri Vanek
Senior QE engineer, OpenJDK QE lead, Mgr.
Red Hat Czech
jvanek at redhat.com    M: +420775390109