<AWT Dev> <Swing Dev> [PATCH] 7168064: SwingUtilities.sharedOwnerFrame multiplies window close event

[Jose Luis Martin]

Hi Anthony, 

Thanks for replying. I comment inline too.

El mar, 21-05-2013 a las 17:49 +0400, Anthony Petrov escribió:
> Hi,
> 
> I'm getting back to this issue. Please see my comments inline:
> On 05/09/2013 02:23 PM, Jose Luis Martin wrote:
> > I update the Test and include two new TestCases:
> >
> > - Test if a Window that has been never shown receive the WINDOW_CLOSED
> > event.
> >
> > - Test if a child window that never has been shown receive the
> > WINDOW_CLOSED event on parent dispose().
> 
> Please note that in JDK we use jtreg as a test harness, not JUnit. See 
> jdk/test/java/awt/ for examples of such tests.

OK, I will write a jtreg test for the issue.

> 
> > With the patch applied the second test fails and the doDispose() method
> > in the child Window is never called so I suppose that the patch is
> > wrong.
> >
> > Looking at code I don't find any reason to call doDispose() on already
> > disposed window or a in a Window that never has been shown. Is there?
> 


> As I explained below, the doDispose() calls methods that may be 
> overridden in user applications (e.g. Window.hide()). If you stop 
> calling this method under certain conditions, you may break those apps 
> for no good reason.

Yes but, this overriden method will be called multiples times just from
one user call to dispose() method. As you can see in the test:

window.dispose() -> (*6)-> disposeAction.run() -> (*6) window.hide().

Is this the correct behavior or should be fixed too? 


> > If not, we can simply test if window is displayable on doDispose() but
> > then the following code will not fire a WINDOW_CLOSED event.
> >
> > new JFrame().dispose();
> >
> > If it's not acceptable, I think that the fix should:
> >
> > - Fire the event and run the dispose action on never shown Windows.
> > - Run the dispose action but not fire the event when re-disposed.
> >
> > Something like:
> >
> > doDispose() {
> > boolean fireClosedEvent = isDisplayable() || hasBeenNeverShown;
> >
> > ...
> >
> > if (fireClosedEvent)
> >     postWindowEvent(WindowEvent.WINDOW_CLOSED);
> > }
> 
> The spec for WINDOW_CLOSED is very vague and reads as:
> 
> > The window closed event. This event is delivered after the window has been closed as the result of a call to dispose.
> 
> The current implementation is such that the event gets sent 
> unconditionally upon every call to dispose(). This doesn't make sense if 
> the window has already been disposed previously, or has never allocated 
> its native resources (i.e. the peer) actually.
> 
> Is this the problem we're trying to resolve?
> 
> If so, then the event must only be sent if the removeNotify() call in 
> doDispose() actually destroys the peer. In other words, if before the 
> call the window has been displayable, and after the call it's no longer 
> displayable, then the event must be sent. It shouldn't be sent under 
> other circumstances.
> 
> Do you see a problem with this solution?

No, it's good for me. It's what I expected to happen.
> 
> What are the advantages of your proposal?

None, I'm only trying too keep the same behavior for  never shown
windows, (that actually fire the event) but not because I think that it
should do. 
> 
> Why do you think we should send the event in case hasBeenNeverShown == 
> true? What would the event mean? Also, please note that 
> hasBeenNeverShown is not equal to "native resources have never been 
> created", because in fact an app may call window.addNotify() directly 
> (which is odd and incorrect, but oh, well..) and this will trigger the 
> creation of a native peer. In this case the WINDOW_CLOSED event must be 
> sent by the dispose() method. Do you agree?

Yes, but not 100%. 

I suppose that the fix you are pointing is:

doDispose() {
   boolean fireClosedEvent = isDisplayable();

   // The following block will run multiplies times from
   // one user window.dispose() call

   (create and run the dispose action in sync with EDT)

  if (fireClosedEvent)
     postWindowEvent(WindowEvent.WINDOW_CLOSED);
}

I'm not 100% sure that the dispose action is idempotent at all.

It is actually calling:

- hide() -> if overriden should be idempotent.
- clearCurrentFocusCycleRootOnHide (actually idempotent).
- removeNotify -> multiplies the HierarchyEvents so HierarchyListeners
should be idempontent (actually not documented).

Should we replay the same fix on the HirearchyEvent in removeNotify
method.?



Best Regards, 

-- Jose Luis Martin.

> 
> --
> best regards,
> Anthony
> 
> >
> > As Anthony suggested.
> >
> >
> > Best Regards,
> >
> > -- Jose Luis Martin
> >
> >
> >
> > El mié, 08-05-2013 a las 16:29 +0400, Anthony Petrov escribió:
> >> Hi,
> >>
> >> This indeed looks like a bug in AWT. I'm BCC'ing swing-dev@, and CC'ing
> >> awt-dev@ instead (please subscribe to this mailing list before posting
> >> to it).
> >>
> >> Here's a question: why do we consider this issue affects child windows
> >> only? If you call dispose() on a regular window several times in a row,
> >> you'll receive that many WINDOW_CLOSED events. Isn't it the real bug
> >> here actually?
> >>
> >> Note that simply guarding a call to dispose()/disposeImpl() with a check
> >> is not the best option because it changes the behavior. If user code
> >> overrides certain methods, they may be stopped being called after your
> >> fix, and this may break that code.
> >>
> >> Since the DisposeAction is executed synchronously anyway, perhaps it
> >> should set a flag based on which the doDispose() method would decide to
> >> send this event or not. What do you think? Also, have you run existing
> >> jtreg tests (in jdk/test/java/awt/) to verify that this fix doesn't
> >> introduce a regression?
> >>
> >> PS. I can't find the bug filed by you in our bug database. I'll file a
> >> new one once we review your fix on this mailing list.
> >>
> >> --
> >> best regards,
> >> Anthony
> >>
> >> On 05/06/2013 12:28 PM, Jose Luis Martin wrote:
> >>> Hello,
> >>>
> >>> I reported this issue one year ago but seems that it has been removed
> >>> from the public bug database and is still not fixed so I try now to
> >>> speak directly with you to see if this really is a bug or not.
> >>>
> >>> The problem that I see is that a child window already disposed is
> >>> re-disposed when the owner frame is disposed, multiplying the
> >>> WINDOW_CLOSED events.
> >>>
> >>> I guess that it could be fixed by testing if the child window is
> >>> displayable before disposing it in the Window dispose action. (May be
> >>> better in the dispose method itself).
> >>>
> >>> I attach patch over last jdk8 repository and a test case for the issue.
> >>>
> >>> Thanks,
> >>>
> >>> Best Regards.
> >>>
> >>>
> >>> -- Jose Luis Martin
> >>>
> >
> >