<AWT Dev> [10] Review request for 8189201: [macosx] NotSerializableException during JFrame with MenuBar serialization

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

<AWT Dev> [10] Review request for 8189201: [macosx] NotSerializableException during JFrame with MenuBar serialization

semyon.sadetsky
Hello,

Please review fix for JDK10:

bug: https://bugs.openjdk.java.net/browse/JDK-8189201

webrev: http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.00

The root cause is that several JFrame fields contain non-serializable
class instances. The fix marks those classes as serializable.

--Semyon

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [10] Review request for 8189201: [macosx] NotSerializableException during JFrame with MenuBar serialization

Sergey Bylokhov
Hi, Semyon.

The AquaMenuBarBorder class is L&F specific, and it should not be
serialized/deserialized. Such information(plus l&f client
properties/listeners/etc) should be removed from the component before
serialization. And during deserialization the L&F of the target system
should be applied.

On 20/10/2017 13:08, Semyon Sadetsky wrote:

> Hello,
>
> Please review fix for JDK10:
>
> bug: https://bugs.openjdk.java.net/browse/JDK-8189201
>
> webrev: http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.00
>
> The root cause is that several JFrame fields contain non-serializable
> class instances. The fix marks those classes as serializable.
>
> --Semyon
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [10] Review request for 8189201: [macosx] NotSerializableException during JFrame with MenuBar serialization

semyon.sadetsky
Hi Sergey,

On 10/20/2017 01:57 PM, Sergey Bylokhov wrote:
> Hi, Semyon.
>
> The AquaMenuBarBorder class is L&F specific, and it should not be
> serialized/deserialized. Such information(plus l&f client
> properties/listeners/etc) should be removed from the component before
> serialization. And during deserialization the L&F of the target system
> should be applied.
That is valid concern. The webrev is updated
http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.01/

--Semyon

>
> On 20/10/2017 13:08, Semyon Sadetsky wrote:
>> Hello,
>>
>> Please review fix for JDK10:
>>
>> bug: https://bugs.openjdk.java.net/browse/JDK-8189201
>>
>> webrev: http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.00
>>
>> The root cause is that several JFrame fields contain non-serializable
>> class instances. The fix marks those classes as serializable.
>>
>> --Semyon
>>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [10] Review request for 8189201: [macosx] NotSerializableException during JFrame with MenuBar serialization

Sergey Bylokhov
On 23/10/2017 12:41, Semyon Sadetsky wrote:
>> The AquaMenuBarBorder class is L&F specific, and it should not be
>> serialized/deserialized. Such information(plus l&f client
>> properties/listeners/etc) should be removed from the component before
>> serialization. And during deserialization the L&F of the target system
>> should be applied.
> That is valid concern. The webrev is updated
> http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.01/

The same is applicable to ScreenMenuPropertyListener as well because it
is also L&F specific.
I assume that the changes in AccessibleAWTComponentHandler/etc are
necessary because somecode installs the listeners on the components,
since the bug is not reproduced in Metal I assume that this listeners
are installed by Aqua, in this case these listeners also should be
removed from the component before serialization.


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [10] Review request for 8189201: [macosx] NotSerializableException during JFrame with MenuBar serialization

semyon.sadetsky
On 10/23/2017 12:58 PM, Sergey Bylokhov wrote:

> On 23/10/2017 12:41, Semyon Sadetsky wrote:
>>> The AquaMenuBarBorder class is L&F specific, and it should not be
>>> serialized/deserialized. Such information(plus l&f client
>>> properties/listeners/etc) should be removed from the component
>>> before serialization. And during deserialization the L&F of the
>>> target system should be applied.
>> That is valid concern. The webrev is updated
>> http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.01/
>
> The same is applicable to ScreenMenuPropertyListener as well because
> it is also L&F specific.
> I assume that the changes in AccessibleAWTComponentHandler/etc are
> necessary because somecode installs the listeners on the components,
> since the bug is not reproduced in Metal I assume that this listeners
> are installed by Aqua, in this case these listeners also should be
> removed from the component before serialization.
ScreenMenu is not a Swing component.

--Semyon

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [10] Review request for 8189201: [macosx] NotSerializableException during JFrame with MenuBar serialization

semyon.sadetsky
Actually ScreenMenu is fully removed in uninstallUI() (line 54 of
AquaMenuBarUI), so there is no need to make its property listener
Serializable, please, ignore the change in ScreenMenuPropertyListener.

--Semyon


On 10/23/2017 01:25 PM, Semyon Sadetsky wrote:

> On 10/23/2017 12:58 PM, Sergey Bylokhov wrote:
>
>> On 23/10/2017 12:41, Semyon Sadetsky wrote:
>>>> The AquaMenuBarBorder class is L&F specific, and it should not be
>>>> serialized/deserialized. Such information(plus l&f client
>>>> properties/listeners/etc) should be removed from the component
>>>> before serialization. And during deserialization the L&F of the
>>>> target system should be applied.
>>> That is valid concern. The webrev is updated
>>> http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.01/
>>
>> The same is applicable to ScreenMenuPropertyListener as well because
>> it is also L&F specific.
>> I assume that the changes in AccessibleAWTComponentHandler/etc are
>> necessary because somecode installs the listeners on the components,
>> since the bug is not reproduced in Metal I assume that this listeners
>> are installed by Aqua, in this case these listeners also should be
>> removed from the component before serialization.
> ScreenMenu is not a Swing component.
>
> --Semyon
>

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [10] Review request for 8189201: [macosx] NotSerializableException during JFrame with MenuBar serialization

semyon.sadetsky
The updated webrev: http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.02/


On 10/23/2017 01:54 PM, Semyon Sadetsky wrote:

> Actually ScreenMenu is fully removed in uninstallUI() (line 54 of
> AquaMenuBarUI), so there is no need to make its property listener
> Serializable, please, ignore the change in ScreenMenuPropertyListener.
>
> --Semyon
>
>
> On 10/23/2017 01:25 PM, Semyon Sadetsky wrote:
>> On 10/23/2017 12:58 PM, Sergey Bylokhov wrote:
>>
>>> On 23/10/2017 12:41, Semyon Sadetsky wrote:
>>>>> The AquaMenuBarBorder class is L&F specific, and it should not be
>>>>> serialized/deserialized. Such information(plus l&f client
>>>>> properties/listeners/etc) should be removed from the component
>>>>> before serialization. And during deserialization the L&F of the
>>>>> target system should be applied.
>>>> That is valid concern. The webrev is updated
>>>> http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.01/
>>>
>>> The same is applicable to ScreenMenuPropertyListener as well because
>>> it is also L&F specific.
>>> I assume that the changes in AccessibleAWTComponentHandler/etc are
>>> necessary because somecode installs the listeners on the components,
>>> since the bug is not reproduced in Metal I assume that this
>>> listeners are installed by Aqua, in this case these listeners also
>>> should be removed from the component before serialization.
>> ScreenMenu is not a Swing component.
>>
>> --Semyon
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [10] Review request for 8189201: [macosx] NotSerializableException during JFrame with MenuBar serialization

Sergey Bylokhov
Some small question while I do a review of other parts.
After the fix the classes which store the listeners like
AccessibleAWTFocusHandler supports serialisation, but most of them are
used in pair with propertyListenersCount which is transient. Should we
serialize the count of listeners?
But from the other point of view all our listeners like
"componentListener", "keyListener" etc are transient and have some
special steps in Component.writeObject().

On 31/10/2017 09:05, Semyon Sadetsky wrote:

> The updated webrev:
> http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.02/
>
>
> On 10/23/2017 01:54 PM, Semyon Sadetsky wrote:
>> Actually ScreenMenu is fully removed in uninstallUI() (line 54 of
>> AquaMenuBarUI), so there is no need to make its property listener
>> Serializable, please, ignore the change in ScreenMenuPropertyListener.
>>
>> --Semyon
>>
>>
>> On 10/23/2017 01:25 PM, Semyon Sadetsky wrote:
>>> On 10/23/2017 12:58 PM, Sergey Bylokhov wrote:
>>>
>>>> On 23/10/2017 12:41, Semyon Sadetsky wrote:
>>>>>> The AquaMenuBarBorder class is L&F specific, and it should not be
>>>>>> serialized/deserialized. Such information(plus l&f client
>>>>>> properties/listeners/etc) should be removed from the component
>>>>>> before serialization. And during deserialization the L&F of the
>>>>>> target system should be applied.
>>>>> That is valid concern. The webrev is updated
>>>>> http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.01/
>>>>
>>>> The same is applicable to ScreenMenuPropertyListener as well because
>>>> it is also L&F specific.
>>>> I assume that the changes in AccessibleAWTComponentHandler/etc are
>>>> necessary because somecode installs the listeners on the components,
>>>> since the bug is not reproduced in Metal I assume that this
>>>> listeners are installed by Aqua, in this case these listeners also
>>>> should be removed from the component before serialization.
>>> ScreenMenu is not a Swing component.
>>>
>>> --Semyon
>>>
>>
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [10] Review request for 8189201: [macosx] NotSerializableException during JFrame with MenuBar serialization

semyon.sadetsky
Hi Sergey,

On 11/02/2017 04:57 PM, Sergey Bylokhov wrote:

> Some small question while I do a review of other parts.
> After the fix the classes which store the listeners like
> AccessibleAWTFocusHandler supports serialisation, but most of them are
> used in pair with propertyListenersCount which is transient. Should we
> serialize the count of listeners?
> But from the other point of view all our listeners like
> "componentListener", "keyListener" etc are transient and have some
> special steps in Component.writeObject().
Yes, it may be necessary to to do something with propertyListenersCount
in writeObject() as well. But this may change the format compatibility.
I have created a separate bug to investigate this:
https://bugs.openjdk.java.net/browse/JDK-8190867

--Semyon

>
> On 31/10/2017 09:05, Semyon Sadetsky wrote:
>> The updated webrev:
>> http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.02/
>>
>>
>> On 10/23/2017 01:54 PM, Semyon Sadetsky wrote:
>>> Actually ScreenMenu is fully removed in uninstallUI() (line 54 of
>>> AquaMenuBarUI), so there is no need to make its property listener
>>> Serializable, please, ignore the change in ScreenMenuPropertyListener.
>>>
>>> --Semyon
>>>
>>>
>>> On 10/23/2017 01:25 PM, Semyon Sadetsky wrote:
>>>> On 10/23/2017 12:58 PM, Sergey Bylokhov wrote:
>>>>
>>>>> On 23/10/2017 12:41, Semyon Sadetsky wrote:
>>>>>>> The AquaMenuBarBorder class is L&F specific, and it should not
>>>>>>> be serialized/deserialized. Such information(plus l&f client
>>>>>>> properties/listeners/etc) should be removed from the component
>>>>>>> before serialization. And during deserialization the L&F of the
>>>>>>> target system should be applied.
>>>>>> That is valid concern. The webrev is updated
>>>>>> http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.01/
>>>>>
>>>>> The same is applicable to ScreenMenuPropertyListener as well
>>>>> because it is also L&F specific.
>>>>> I assume that the changes in AccessibleAWTComponentHandler/etc are
>>>>> necessary because somecode installs the listeners on the
>>>>> components, since the bug is not reproduced in Metal I assume that
>>>>> this listeners are installed by Aqua, in this case these listeners
>>>>> also should be removed from the component before serialization.
>>>> ScreenMenu is not a Swing component.
>>>>
>>>> --Semyon
>>>>
>>>
>>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [10] Review request for 8189201: [macosx] NotSerializableException during JFrame with MenuBar serialization

Sergey Bylokhov
On 07/11/2017 08:48, Semyon Sadetsky wrote:
>> But from the other point of view all our listeners like
>> "componentListener", "keyListener" etc are transient and have some
>> special steps in Component.writeObject().
> Yes, it may be necessary to to do something with propertyListenersCount
> in writeObject() as well. But this may change the format compatibility.
> I have created a separate bug to investigate this:
> https://bugs.openjdk.java.net/browse/JDK-8190867

But it does not explain why other containers for listeners marked as
transient. I guess the reason is in this part of spec in
Component.writeObject():
      * Writes default serializable fields to stream.  Writes
      * a variety of serializable listeners as optional data.
      * The non-serializable listeners are detected and
      * no attempt is made to serialize them.

This is why all of them marked as transient and during serialization we
iterates over them and save only listeners which implements Serializable
interface.

>
> --Semyon
>>
>> On 31/10/2017 09:05, Semyon Sadetsky wrote:
>>> The updated webrev:
>>> http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.02/
>>>
>>>
>>> On 10/23/2017 01:54 PM, Semyon Sadetsky wrote:
>>>> Actually ScreenMenu is fully removed in uninstallUI() (line 54 of
>>>> AquaMenuBarUI), so there is no need to make its property listener
>>>> Serializable, please, ignore the change in ScreenMenuPropertyListener.
>>>>
>>>> --Semyon
>>>>
>>>>
>>>> On 10/23/2017 01:25 PM, Semyon Sadetsky wrote:
>>>>> On 10/23/2017 12:58 PM, Sergey Bylokhov wrote:
>>>>>
>>>>>> On 23/10/2017 12:41, Semyon Sadetsky wrote:
>>>>>>>> The AquaMenuBarBorder class is L&F specific, and it should not
>>>>>>>> be serialized/deserialized. Such information(plus l&f client
>>>>>>>> properties/listeners/etc) should be removed from the component
>>>>>>>> before serialization. And during deserialization the L&F of the
>>>>>>>> target system should be applied.
>>>>>>> That is valid concern. The webrev is updated
>>>>>>> http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.01/
>>>>>>
>>>>>> The same is applicable to ScreenMenuPropertyListener as well
>>>>>> because it is also L&F specific.
>>>>>> I assume that the changes in AccessibleAWTComponentHandler/etc are
>>>>>> necessary because somecode installs the listeners on the
>>>>>> components, since the bug is not reproduced in Metal I assume that
>>>>>> this listeners are installed by Aqua, in this case these listeners
>>>>>> also should be removed from the component before serialization.
>>>>> ScreenMenu is not a Swing component.
>>>>>
>>>>> --Semyon
>>>>>
>>>>
>>>
>>
>>
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [10] Review request for 8189201: [macosx] NotSerializableException during JFrame with MenuBar serialization

semyon.sadetsky


On 11/07/2017 10:21 AM, Sergey Bylokhov wrote:

> On 07/11/2017 08:48, Semyon Sadetsky wrote:
>>> But from the other point of view all our listeners like
>>> "componentListener", "keyListener" etc are transient and have some
>>> special steps in Component.writeObject().
>> Yes, it may be necessary to to do something with
>> propertyListenersCount in writeObject() as well. But this may change
>> the format compatibility. I have created a separate bug to
>> investigate this: https://bugs.openjdk.java.net/browse/JDK-8190867
>
> But it does not explain why other containers for listeners marked as
> transient. I guess the reason is in this part of spec in
> Component.writeObject():
>      * Writes default serializable fields to stream.  Writes
>      * a variety of serializable listeners as optional data.
>      * The non-serializable listeners are detected and
>      * no attempt is made to serialize them.
>
> This is why all of them marked as transient and during serialization
> we iterates over them and save only listeners which implements
> Serializable interface.
I did get this. This spec is unrelated to the current issue. The current
issue is very simple:

AccessibleAWTComponent class is Serializable but its fields
accessibleAWTComponentHandler and  accessibleAWTFocusHandler are not.

This violates the Java serialization policy and causes exception in JDK
code. The proposed fix eliminates this issue. The fields cannot be
changed to transient because this would break the logic after
deserialization of the component.

>
>>
>> --Semyon
>>>
>>> On 31/10/2017 09:05, Semyon Sadetsky wrote:
>>>> The updated webrev:
>>>> http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.02/
>>>>
>>>>
>>>> On 10/23/2017 01:54 PM, Semyon Sadetsky wrote:
>>>>> Actually ScreenMenu is fully removed in uninstallUI() (line 54 of
>>>>> AquaMenuBarUI), so there is no need to make its property listener
>>>>> Serializable, please, ignore the change in
>>>>> ScreenMenuPropertyListener.
>>>>>
>>>>> --Semyon
>>>>>
>>>>>
>>>>> On 10/23/2017 01:25 PM, Semyon Sadetsky wrote:
>>>>>> On 10/23/2017 12:58 PM, Sergey Bylokhov wrote:
>>>>>>
>>>>>>> On 23/10/2017 12:41, Semyon Sadetsky wrote:
>>>>>>>>> The AquaMenuBarBorder class is L&F specific, and it should not
>>>>>>>>> be serialized/deserialized. Such information(plus l&f client
>>>>>>>>> properties/listeners/etc) should be removed from the component
>>>>>>>>> before serialization. And during deserialization the L&F of
>>>>>>>>> the target system should be applied.
>>>>>>>> That is valid concern. The webrev is updated
>>>>>>>> http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.01/
>>>>>>>
>>>>>>> The same is applicable to ScreenMenuPropertyListener as well
>>>>>>> because it is also L&F specific.
>>>>>>> I assume that the changes in AccessibleAWTComponentHandler/etc
>>>>>>> are necessary because somecode installs the listeners on the
>>>>>>> components, since the bug is not reproduced in Metal I assume
>>>>>>> that this listeners are installed by Aqua, in this case these
>>>>>>> listeners also should be removed from the component before
>>>>>>> serialization.
>>>>>> ScreenMenu is not a Swing component.
>>>>>>
>>>>>> --Semyon
>>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [10] Review request for 8189201: [macosx] NotSerializableException during JFrame with MenuBar serialization

Sergey Bylokhov
On 07/11/2017 10:42, Semyon Sadetsky wrote:

> I did get this. This spec is unrelated to the current issue. The current
> issue is very simple:
>
> AccessibleAWTComponent class is Serializable but its fields
> accessibleAWTComponentHandler and  accessibleAWTFocusHandler are not.
>
> This violates the Java serialization policy and causes exception in JDK
> code. The proposed fix eliminates this issue. The fields cannot be
> changed to transient because this would break the logic after
> deserialization of the component.

Ok, its up to you.

>>
>>>
>>> --Semyon
>>>>
>>>> On 31/10/2017 09:05, Semyon Sadetsky wrote:
>>>>> The updated webrev:
>>>>> http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.02/
>>>>>
>>>>>
>>>>> On 10/23/2017 01:54 PM, Semyon Sadetsky wrote:
>>>>>> Actually ScreenMenu is fully removed in uninstallUI() (line 54 of
>>>>>> AquaMenuBarUI), so there is no need to make its property listener
>>>>>> Serializable, please, ignore the change in
>>>>>> ScreenMenuPropertyListener.
>>>>>>
>>>>>> --Semyon
>>>>>>
>>>>>>
>>>>>> On 10/23/2017 01:25 PM, Semyon Sadetsky wrote:
>>>>>>> On 10/23/2017 12:58 PM, Sergey Bylokhov wrote:
>>>>>>>
>>>>>>>> On 23/10/2017 12:41, Semyon Sadetsky wrote:
>>>>>>>>>> The AquaMenuBarBorder class is L&F specific, and it should not
>>>>>>>>>> be serialized/deserialized. Such information(plus l&f client
>>>>>>>>>> properties/listeners/etc) should be removed from the component
>>>>>>>>>> before serialization. And during deserialization the L&F of
>>>>>>>>>> the target system should be applied.
>>>>>>>>> That is valid concern. The webrev is updated
>>>>>>>>> http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.01/
>>>>>>>>
>>>>>>>> The same is applicable to ScreenMenuPropertyListener as well
>>>>>>>> because it is also L&F specific.
>>>>>>>> I assume that the changes in AccessibleAWTComponentHandler/etc
>>>>>>>> are necessary because somecode installs the listeners on the
>>>>>>>> components, since the bug is not reproduced in Metal I assume
>>>>>>>> that this listeners are installed by Aqua, in this case these
>>>>>>>> listeners also should be removed from the component before
>>>>>>>> serialization.
>>>>>>> ScreenMenu is not a Swing component.
>>>>>>>
>>>>>>> --Semyon
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [10] Review request for 8189201: [macosx] NotSerializableException during JFrame with MenuBar serialization

Philip Race
In reply to this post by semyon.sadetsky
 > The fields cannot be changed to transient because this would break
the logic after deserialization of the component.

Can you explain what logic would be broken since changing these to
transient looks tempting ..

It can't be a problem for serialisation of actual non-null values since
demonstrably that
can't ever have worked.

If there is broken logic for when they are null, what is it ?

-phil.

On 11/07/2017 10:42 AM, Semyon Sadetsky wrote:

>
>
> On 11/07/2017 10:21 AM, Sergey Bylokhov wrote:
>> On 07/11/2017 08:48, Semyon Sadetsky wrote:
>>>> But from the other point of view all our listeners like
>>>> "componentListener", "keyListener" etc are transient and have some
>>>> special steps in Component.writeObject().
>>> Yes, it may be necessary to to do something with
>>> propertyListenersCount in writeObject() as well. But this may change
>>> the format compatibility. I have created a separate bug to
>>> investigate this: https://bugs.openjdk.java.net/browse/JDK-8190867
>>
>> But it does not explain why other containers for listeners marked as
>> transient. I guess the reason is in this part of spec in
>> Component.writeObject():
>>      * Writes default serializable fields to stream.  Writes
>>      * a variety of serializable listeners as optional data.
>>      * The non-serializable listeners are detected and
>>      * no attempt is made to serialize them.
>>
>> This is why all of them marked as transient and during serialization
>> we iterates over them and save only listeners which implements
>> Serializable interface.
> I did get this. This spec is unrelated to the current issue. The
> current issue is very simple:
>
> AccessibleAWTComponent class is Serializable but its fields
> accessibleAWTComponentHandler and  accessibleAWTFocusHandler are not.
>
> This violates the Java serialization policy and causes exception in
> JDK code. The proposed fix eliminates this issue. The fields cannot be
> changed to transient because this would break the logic after
> deserialization of the component.
>>
>>>
>>> --Semyon
>>>>
>>>> On 31/10/2017 09:05, Semyon Sadetsky wrote:
>>>>> The updated webrev:
>>>>> http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.02/
>>>>>
>>>>>
>>>>> On 10/23/2017 01:54 PM, Semyon Sadetsky wrote:
>>>>>> Actually ScreenMenu is fully removed in uninstallUI() (line 54 of
>>>>>> AquaMenuBarUI), so there is no need to make its property listener
>>>>>> Serializable, please, ignore the change in
>>>>>> ScreenMenuPropertyListener.
>>>>>>
>>>>>> --Semyon
>>>>>>
>>>>>>
>>>>>> On 10/23/2017 01:25 PM, Semyon Sadetsky wrote:
>>>>>>> On 10/23/2017 12:58 PM, Sergey Bylokhov wrote:
>>>>>>>
>>>>>>>> On 23/10/2017 12:41, Semyon Sadetsky wrote:
>>>>>>>>>> The AquaMenuBarBorder class is L&F specific, and it should
>>>>>>>>>> not be serialized/deserialized. Such information(plus l&f
>>>>>>>>>> client properties/listeners/etc) should be removed from the
>>>>>>>>>> component before serialization. And during deserialization
>>>>>>>>>> the L&F of the target system should be applied.
>>>>>>>>> That is valid concern. The webrev is updated
>>>>>>>>> http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.01/
>>>>>>>>
>>>>>>>> The same is applicable to ScreenMenuPropertyListener as well
>>>>>>>> because it is also L&F specific.
>>>>>>>> I assume that the changes in AccessibleAWTComponentHandler/etc
>>>>>>>> are necessary because somecode installs the listeners on the
>>>>>>>> components, since the bug is not reproduced in Metal I assume
>>>>>>>> that this listeners are installed by Aqua, in this case these
>>>>>>>> listeners also should be removed from the component before
>>>>>>>> serialization.
>>>>>>> ScreenMenu is not a Swing component.
>>>>>>>
>>>>>>> --Semyon
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [10] Review request for 8189201: [macosx] NotSerializableException during JFrame with MenuBar serialization

semyon.sadetsky
On 11/07/2017 12:54 PM, Phil Race wrote:
> > The fields cannot be changed to transient because this would break
> the logic after deserialization of the component.
>
> Can you explain what logic would be broken since changing these to
> transient looks tempting ..

Some evnts would not reach the accessibility listener.

>
> It can't be a problem for serialisation of actual non-null values
> since demonstrably that
> can't ever have worked.
They can be involved if the listener is added.
>
> If there is broken logic for when they are null, what is it ?
AccessibleAWTComponentHandler and AccessibleAWTFocusHandler rethrow
events that a necessary for AccessibleAWTComponent listeners. If those
objects are absent the listener will not receive the events. Since this
is a public API it only means that the contract is not met.

--Semyon

>
> -phil.
>
> On 11/07/2017 10:42 AM, Semyon Sadetsky wrote:
>>
>>
>> On 11/07/2017 10:21 AM, Sergey Bylokhov wrote:
>>> On 07/11/2017 08:48, Semyon Sadetsky wrote:
>>>>> But from the other point of view all our listeners like
>>>>> "componentListener", "keyListener" etc are transient and have some
>>>>> special steps in Component.writeObject().
>>>> Yes, it may be necessary to to do something with
>>>> propertyListenersCount in writeObject() as well. But this may
>>>> change the format compatibility. I have created a separate bug to
>>>> investigate this: https://bugs.openjdk.java.net/browse/JDK-8190867
>>>
>>> But it does not explain why other containers for listeners marked as
>>> transient. I guess the reason is in this part of spec in
>>> Component.writeObject():
>>>      * Writes default serializable fields to stream.  Writes
>>>      * a variety of serializable listeners as optional data.
>>>      * The non-serializable listeners are detected and
>>>      * no attempt is made to serialize them.
>>>
>>> This is why all of them marked as transient and during serialization
>>> we iterates over them and save only listeners which implements
>>> Serializable interface.
>> I did get this. This spec is unrelated to the current issue. The
>> current issue is very simple:
>>
>> AccessibleAWTComponent class is Serializable but its fields
>> accessibleAWTComponentHandler and  accessibleAWTFocusHandler are not.
>>
>> This violates the Java serialization policy and causes exception in
>> JDK code. The proposed fix eliminates this issue. The fields cannot
>> be changed to transient because this would break the logic after
>> deserialization of the component.
>>>
>>>>
>>>> --Semyon
>>>>>
>>>>> On 31/10/2017 09:05, Semyon Sadetsky wrote:
>>>>>> The updated webrev:
>>>>>> http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.02/
>>>>>>
>>>>>>
>>>>>> On 10/23/2017 01:54 PM, Semyon Sadetsky wrote:
>>>>>>> Actually ScreenMenu is fully removed in uninstallUI() (line 54
>>>>>>> of AquaMenuBarUI), so there is no need to make its property
>>>>>>> listener Serializable, please, ignore the change in
>>>>>>> ScreenMenuPropertyListener.
>>>>>>>
>>>>>>> --Semyon
>>>>>>>
>>>>>>>
>>>>>>> On 10/23/2017 01:25 PM, Semyon Sadetsky wrote:
>>>>>>>> On 10/23/2017 12:58 PM, Sergey Bylokhov wrote:
>>>>>>>>
>>>>>>>>> On 23/10/2017 12:41, Semyon Sadetsky wrote:
>>>>>>>>>>> The AquaMenuBarBorder class is L&F specific, and it should
>>>>>>>>>>> not be serialized/deserialized. Such information(plus l&f
>>>>>>>>>>> client properties/listeners/etc) should be removed from the
>>>>>>>>>>> component before serialization. And during deserialization
>>>>>>>>>>> the L&F of the target system should be applied.
>>>>>>>>>> That is valid concern. The webrev is updated
>>>>>>>>>> http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.01/
>>>>>>>>>
>>>>>>>>> The same is applicable to ScreenMenuPropertyListener as well
>>>>>>>>> because it is also L&F specific.
>>>>>>>>> I assume that the changes in AccessibleAWTComponentHandler/etc
>>>>>>>>> are necessary because somecode installs the listeners on the
>>>>>>>>> components, since the bug is not reproduced in Metal I assume
>>>>>>>>> that this listeners are installed by Aqua, in this case these
>>>>>>>>> listeners also should be removed from the component before
>>>>>>>>> serialization.
>>>>>>>> ScreenMenu is not a Swing component.
>>>>>>>>
>>>>>>>> --Semyon
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [10] Review request for 8189201: [macosx] NotSerializableException during JFrame with MenuBar serialization

Philip Race

Ok but you need a CSR first for the API changes .. and so this
fix cannot be backported.

-phil.

On 11/08/2017 10:35 AM, Semyon Sadetsky wrote:

> On 11/07/2017 12:54 PM, Phil Race wrote:
>> > The fields cannot be changed to transient because this would break
>> the logic after deserialization of the component.
>>
>> Can you explain what logic would be broken since changing these to
>> transient looks tempting ..
>
> Some evnts would not reach the accessibility listener.
>
>>
>> It can't be a problem for serialisation of actual non-null values
>> since demonstrably that
>> can't ever have worked.
> They can be involved if the listener is added.
>>
>> If there is broken logic for when they are null, what is it ?
> AccessibleAWTComponentHandler and AccessibleAWTFocusHandler rethrow
> events that a necessary for AccessibleAWTComponent listeners. If those
> objects are absent the listener will not receive the events. Since
> this is a public API it only means that the contract is not met.
>
> --Semyon
>>
>> -phil.
>>
>> On 11/07/2017 10:42 AM, Semyon Sadetsky wrote:
>>>
>>>
>>> On 11/07/2017 10:21 AM, Sergey Bylokhov wrote:
>>>> On 07/11/2017 08:48, Semyon Sadetsky wrote:
>>>>>> But from the other point of view all our listeners like
>>>>>> "componentListener", "keyListener" etc are transient and have
>>>>>> some special steps in Component.writeObject().
>>>>> Yes, it may be necessary to to do something with
>>>>> propertyListenersCount in writeObject() as well. But this may
>>>>> change the format compatibility. I have created a separate bug to
>>>>> investigate this: https://bugs.openjdk.java.net/browse/JDK-8190867
>>>>
>>>> But it does not explain why other containers for listeners marked
>>>> as transient. I guess the reason is in this part of spec in
>>>> Component.writeObject():
>>>>      * Writes default serializable fields to stream.  Writes
>>>>      * a variety of serializable listeners as optional data.
>>>>      * The non-serializable listeners are detected and
>>>>      * no attempt is made to serialize them.
>>>>
>>>> This is why all of them marked as transient and during
>>>> serialization we iterates over them and save only listeners which
>>>> implements Serializable interface.
>>> I did get this. This spec is unrelated to the current issue. The
>>> current issue is very simple:
>>>
>>> AccessibleAWTComponent class is Serializable but its fields
>>> accessibleAWTComponentHandler and  accessibleAWTFocusHandler are not.
>>>
>>> This violates the Java serialization policy and causes exception in
>>> JDK code. The proposed fix eliminates this issue. The fields cannot
>>> be changed to transient because this would break the logic after
>>> deserialization of the component.
>>>>
>>>>>
>>>>> --Semyon
>>>>>>
>>>>>> On 31/10/2017 09:05, Semyon Sadetsky wrote:
>>>>>>> The updated webrev:
>>>>>>> http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.02/
>>>>>>>
>>>>>>>
>>>>>>> On 10/23/2017 01:54 PM, Semyon Sadetsky wrote:
>>>>>>>> Actually ScreenMenu is fully removed in uninstallUI() (line 54
>>>>>>>> of AquaMenuBarUI), so there is no need to make its property
>>>>>>>> listener Serializable, please, ignore the change in
>>>>>>>> ScreenMenuPropertyListener.
>>>>>>>>
>>>>>>>> --Semyon
>>>>>>>>
>>>>>>>>
>>>>>>>> On 10/23/2017 01:25 PM, Semyon Sadetsky wrote:
>>>>>>>>> On 10/23/2017 12:58 PM, Sergey Bylokhov wrote:
>>>>>>>>>
>>>>>>>>>> On 23/10/2017 12:41, Semyon Sadetsky wrote:
>>>>>>>>>>>> The AquaMenuBarBorder class is L&F specific, and it should
>>>>>>>>>>>> not be serialized/deserialized. Such information(plus l&f
>>>>>>>>>>>> client properties/listeners/etc) should be removed from the
>>>>>>>>>>>> component before serialization. And during deserialization
>>>>>>>>>>>> the L&F of the target system should be applied.
>>>>>>>>>>> That is valid concern. The webrev is updated
>>>>>>>>>>> http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.01/
>>>>>>>>>>
>>>>>>>>>> The same is applicable to ScreenMenuPropertyListener as well
>>>>>>>>>> because it is also L&F specific.
>>>>>>>>>> I assume that the changes in
>>>>>>>>>> AccessibleAWTComponentHandler/etc are necessary because
>>>>>>>>>> somecode installs the listeners on the components, since the
>>>>>>>>>> bug is not reproduced in Metal I assume that this listeners
>>>>>>>>>> are installed by Aqua, in this case these listeners also
>>>>>>>>>> should be removed from the component before serialization.
>>>>>>>>> ScreenMenu is not a Swing component.
>>>>>>>>>
>>>>>>>>> --Semyon
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [10] Review request for 8189201: [macosx] NotSerializableException during JFrame with MenuBar serialization

semyon.sadetsky
Thanks. The CSR was created.

--Semyon


On 11/28/2017 12:38 PM, Phil Race wrote:

>
> Ok but you need a CSR first for the API changes .. and so this
> fix cannot be backported.
>
> -phil.
>
> On 11/08/2017 10:35 AM, Semyon Sadetsky wrote:
>> On 11/07/2017 12:54 PM, Phil Race wrote:
>>> > The fields cannot be changed to transient because this would break
>>> the logic after deserialization of the component.
>>>
>>> Can you explain what logic would be broken since changing these to
>>> transient looks tempting ..
>>
>> Some evnts would not reach the accessibility listener.
>>
>>>
>>> It can't be a problem for serialisation of actual non-null values
>>> since demonstrably that
>>> can't ever have worked.
>> They can be involved if the listener is added.
>>>
>>> If there is broken logic for when they are null, what is it ?
>> AccessibleAWTComponentHandler and AccessibleAWTFocusHandler rethrow
>> events that a necessary for AccessibleAWTComponent listeners. If
>> those objects are absent the listener will not receive the events.
>> Since this is a public API it only means that the contract is not met.
>>
>> --Semyon
>>>
>>> -phil.
>>>
>>> On 11/07/2017 10:42 AM, Semyon Sadetsky wrote:
>>>>
>>>>
>>>> On 11/07/2017 10:21 AM, Sergey Bylokhov wrote:
>>>>> On 07/11/2017 08:48, Semyon Sadetsky wrote:
>>>>>>> But from the other point of view all our listeners like
>>>>>>> "componentListener", "keyListener" etc are transient and have
>>>>>>> some special steps in Component.writeObject().
>>>>>> Yes, it may be necessary to to do something with
>>>>>> propertyListenersCount in writeObject() as well. But this may
>>>>>> change the format compatibility. I have created a separate bug to
>>>>>> investigate this: https://bugs.openjdk.java.net/browse/JDK-8190867
>>>>>
>>>>> But it does not explain why other containers for listeners marked
>>>>> as transient. I guess the reason is in this part of spec in
>>>>> Component.writeObject():
>>>>>      * Writes default serializable fields to stream. Writes
>>>>>      * a variety of serializable listeners as optional data.
>>>>>      * The non-serializable listeners are detected and
>>>>>      * no attempt is made to serialize them.
>>>>>
>>>>> This is why all of them marked as transient and during
>>>>> serialization we iterates over them and save only listeners which
>>>>> implements Serializable interface.
>>>> I did get this. This spec is unrelated to the current issue. The
>>>> current issue is very simple:
>>>>
>>>> AccessibleAWTComponent class is Serializable but its fields
>>>> accessibleAWTComponentHandler and  accessibleAWTFocusHandler are not.
>>>>
>>>> This violates the Java serialization policy and causes exception in
>>>> JDK code. The proposed fix eliminates this issue. The fields cannot
>>>> be changed to transient because this would break the logic after
>>>> deserialization of the component.
>>>>>
>>>>>>
>>>>>> --Semyon
>>>>>>>
>>>>>>> On 31/10/2017 09:05, Semyon Sadetsky wrote:
>>>>>>>> The updated webrev:
>>>>>>>> http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.02/
>>>>>>>>
>>>>>>>>
>>>>>>>> On 10/23/2017 01:54 PM, Semyon Sadetsky wrote:
>>>>>>>>> Actually ScreenMenu is fully removed in uninstallUI() (line 54
>>>>>>>>> of AquaMenuBarUI), so there is no need to make its property
>>>>>>>>> listener Serializable, please, ignore the change in
>>>>>>>>> ScreenMenuPropertyListener.
>>>>>>>>>
>>>>>>>>> --Semyon
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 10/23/2017 01:25 PM, Semyon Sadetsky wrote:
>>>>>>>>>> On 10/23/2017 12:58 PM, Sergey Bylokhov wrote:
>>>>>>>>>>
>>>>>>>>>>> On 23/10/2017 12:41, Semyon Sadetsky wrote:
>>>>>>>>>>>>> The AquaMenuBarBorder class is L&F specific, and it should
>>>>>>>>>>>>> not be serialized/deserialized. Such information(plus l&f
>>>>>>>>>>>>> client properties/listeners/etc) should be removed from
>>>>>>>>>>>>> the component before serialization. And during
>>>>>>>>>>>>> deserialization the L&F of the target system should be
>>>>>>>>>>>>> applied.
>>>>>>>>>>>> That is valid concern. The webrev is updated
>>>>>>>>>>>> http://cr.openjdk.java.net/~ssadetsky/8189201/webrev.01/
>>>>>>>>>>>
>>>>>>>>>>> The same is applicable to ScreenMenuPropertyListener as well
>>>>>>>>>>> because it is also L&F specific.
>>>>>>>>>>> I assume that the changes in
>>>>>>>>>>> AccessibleAWTComponentHandler/etc are necessary because
>>>>>>>>>>> somecode installs the listeners on the components, since the
>>>>>>>>>>> bug is not reproduced in Metal I assume that this listeners
>>>>>>>>>>> are installed by Aqua, in this case these listeners also
>>>>>>>>>>> should be removed from the component before serialization.
>>>>>>>>>> ScreenMenu is not a Swing component.
>>>>>>>>>>
>>>>>>>>>> --Semyon
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>