<AWT Dev> [12] RFR : JDK-8198000 : java/awt/List/EmptyListEventTest/EmptyListEventTest.java debug assert on Windows
Phil Race
philip.race at oracle.com
Thu Nov 8 17:33:03 UTC 2018
Ok +1
-phil.
On 11/8/18 9:27 AM, Ambarish Rapte wrote:
>
> Hi Phil,
>
> Thanks for the guiding over discussion,
>
> Please take a look at the updated webrev :
> http://cr.openjdk.java.net/~arapte/8198000/webrev.05/
>
> Regards,
>
> Ambarish
>
> *From:*Ambarish Rapte
> *Sent:* Wednesday, November 07, 2018 10:49 PM
> *To:* Philip Race <philip.race at oracle.com>
> *Cc:* awt-dev at openjdk.java.net
> *Subject:* Re: <AWT Dev> [12] RFR : JDK-8198000 :
> java/awt/List/EmptyListEventTest/EmptyListEventTest.java debug assert
> on Windows
>
> Hi Phil,
>
> > I don't think so. I already pointed out that it is apparently
> returning -1 in your test
> > when HIWORD is 1. I am pretty sure it isn't expecting you to
> interpret 0xFFFF stored
> > in 16 bits as positive.
>
> The value 0xFFFF is -1 when saved into a signed short and 65535 when
> assigned to unsigned short.
>
> I was trying to find any supporting documentation or source code from
> MSDN. But cannot be certain of the behavior based on documentation.
>
> So I also think that we can go ahead with the earlier version of fix
> as you suggested. : http://cr.openjdk.java.net/~arapte/8198000/webrev.04/
>
> Please take a look at the fix, The webrev has fix only for JDK-8198000.
> The fix change for issue #2 i.e. cmath::pow function is removed from
> webrev.04
>
> > These are platform UI components. They can behave differently w.r.t
> things like
> > how they respond to mouse clicks, all dependent on what the platform
> norms are.
> > I see no reason to go out of our way to "make windows mouse clicks
> behave like mac".
>
> This looks good to me as well. With the changes in webrev.04, an item
> gets selected only when mouse is clicked on the item.
>
> > Not understanding you. How can you tell if 0x0001 is supposed to be
> "1" or "65536" ?
>
> Sorry for not explaining the fix in earlier email with webrev.03.
>
> I have removed this change from the webrev.
>
> I should not have mixed fix of the two issues. If the issue #2 needs
> to be addressed, I request if we can work on this issue separately.
>
> > So if the user clicks outside we do nothing. What is wrong with that ?
>
> I too see nothing wrong in this behavior. Mouse click outside the
> client area can be neglected under this fix.
>
> Please take a look at the updated webrev.04 which includes fix only
> for JDK-8198000
>
> Regards,
>
> Ambarish
>
> *From:*Philip Race
> *Sent:* Tuesday, November 06, 2018 10:21 PM
> *To:* Ambarish Rapte <ambarish.rapte at oracle.com
> <mailto:ambarish.rapte at oracle.com>>
> *Cc:* awt-dev at openjdk.java.net <mailto:awt-dev at openjdk.java.net>
> *Subject:* Re: <AWT Dev> [12] RFR : JDK-8198000 :
> java/awt/List/EmptyListEventTest/EmptyListEventTest.java debug assert
> on Windows
>
> On 11/5/18 8:51 AM, Ambarish Rapte wrote:
>
> Hi Phil,
>
> The little abstraction or lack of clarity in documentation is
> leading us to make some assumptions.
>
> Just trying to connect the pieces,
>
> I referred these two doc pages to learn LOWORD behavior:
>
> 1. LOWORD :
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms632659(v=vs.85).aspx
> <https://msdn.microsoft.com/en-us/library/windows/desktop/ms632659%28v=vs.85%29.aspx>
> 2. WORD :
> https://docs.microsoft.com/en-us/windows/desktop/winprog/windows-data-types
>
> And the documentation of LB_ITEMFROMPOINT does not mention LB_ERR(-1)
>
> Based on these documentation, it looks safe to conclude that
> LOWORD would be positive.
>
>
> I don't think so. I already pointed out that it is apparently
> returning -1 in your test
> when HIWORD is 1. I am pretty sure it isn't expecting you to interpret
> 0xFFFF stored
> in 16 bits as positive.
>
> Issue #1 :
>
> Yes, the HIWORD is 1, when the mouse is clicked inside the list’s
> client area but not on the item.
>
> Once we separate the HIWORD and LOWORD, LOWORD gives correct index
> of the last item, and this issue gets solved.
>
> Checking if HIWORD is 1 can be additional check. But as we extract
> correct index, this check can be ignored for allowing selection.
>
> Or the other way, we can use HIWORD to avoid selection/deselection
> when it is 1.
>
> Additionally, The behavior on Ubuntu and mac is different,
>
> Ubuntu: Last item does not get selected & it also does not select
> any other item. User must click on the item to select or deselect.
>
> Mac: Selects the last item
>
> Windows: Focused item gets selected / deselected on multiple
> clicks, 3-4
>
> * After this fix[Allowing selection], Mac and Windows will have
> same behavior.
>
>
> These are platform UI components. They can behave differently w.r.t
> things like
> how they respond to mouse clicks, all dependent on what the platform
> norms are.
> I see no reason to go out of our way to "make windows mouse clicks
> behave like mac".
>
>
> *
>
> Issue #2 :
>
> This looks a very corner case of list of size more than 65535
> size, but the item selection and deselection works fine with keyboard.
>
> Also List behaves correctly on Ubuntu and Mac.
>
> Just for a look, I am including a fix for this issue in webrev.03:
> http://cr.openjdk.java.net/~arapte/8198000/webrev.03/
> <http://cr.openjdk.java.net/%7Earapte/8198000/webrev.03/>
>
> As you have already mentioned we should not really change a long
> running behavior, but I could not hold on from suggestion.
>
> If you think this fix is not needed, I shall just change/remove it.
>
>
> Not understanding you. How can you tell if 0x0001 is supposed to be
> "1" or "65536" ?
>
> I also don't like std::pow .. it is unnecessary here.
>
> Not withstanding the lack of clarity on how you are claiming to know that,
> I was expecting a fix to look more like
>
> LONG count = GetCount();
> if (count > 0) {
> LONG item = static_cast<LONG>(SendListMessage(LB_ITEMFROMPOINT, 0, msg->lParam));
> if (HIWORD(item) == 0) {
> item = LOWORD(item);
> if (item > 0 && item < GetCount()) {
> ...
>
> So if the user clicks outside we do nothing. What is wrong with that ?
> -phil.
>
>
>
> Regards,
>
> Ambarish
>
> *From:*Philip Race
> *Sent:* Saturday, November 03, 2018 10:59 PM
> *To:* Ambarish Rapte <ambarish.rapte at oracle.com>
> <mailto:ambarish.rapte at oracle.com>
> *Cc:* awt-dev at openjdk.java.net <mailto:awt-dev at openjdk.java.net>
> *Subject:* Re: <AWT Dev> [12] RFR : JDK-8198000 :
> java/awt/List/EmptyListEventTest/EmptyListEventTest.java debug
> assert on Windows
>
> - I am not sure you can assume the LOWORD value is non-negative.
> It seems to me that the 0XFFFF we got back is meant to be
> interpreted as "-1"
> which is what I wrote below.
> I do note that it appears that in issue #2 you are seeing that
> up to 65534 items
> may be allowed and it wraps if you add more than 65535 ..
> So *perhaps* we can interpret 0xFFFF as meaning positive 65535
> but I think
> *only* if HIWORD is "0".
>
> - Issue #1 : I am not sure that a "1" in HIWORD automatically
> means it is off the
> end of the list ... just "outside the client area". I think if
> HIWORD is 1 we just
> bail don't we ?
> Then you don't have to worry about whether 0xFFFF meant -1 or 65536
>
> - Issue #2 : There is always *some* limit in cases like this.
> 32767 (2^15-1) or 65535 (2^16-1)
> are very typical in these platform APIs. Often the platform
> doesn't explicitly document it
> and you have to infer it from the data type. I think it was all
> very moot when these APIs
> were designed because you'd run out of memory before you could get
> that many items :-)
> I'd be surprised if there were not already some open bug pointing
> out that we accept "int"
> for index and don't have any feedback when exceeding platform limits.
> From a compatibility point of view, I don't think it is worth
> doing anything that would
> break ancient applications to provide that feedback.
>
> -phil.
>
>
>
> On 11/2/18, 10:23 AM, Ambarish Rapte wrote:
>
> Hi Phil,
>
> Thanks for guiding with the documentation.
>
> The fix is modified after a relook at the documentation, and
> observed two issues [mentioned below].
>
> webrev: http://cr.openjdk.java.net/~arapte/8198000/webrev.02
> <http://cr.openjdk.java.net/%7Earapte/8198000/webrev.02>
>
> Fix Justification:
>
> LB_ITEMFROMPOINT
> documentation(https://docs.microsoft.com/en-us/windows/desktop/controls/lb-itemfrompoint):
>
> 1. Message LB_ITEMFROMPOINT returns a LONG value.
> 2. There is no mention of LB_ERR as return value, like it is
> mentioned clearly for some of the other messages as
> LB_GETCOUNT, LB_SETITEMHEIGHT, LB_GETTEXT, LB_GETCURSEL.
>
> * So the existing comparison against LB_ERR is incorrect.
>
> 3. The two parts LOWORD and HIWORD of return value are of
> type WORD(unsigned short), so the return value can never
> be negative.
>
> * It is another reason to not to compare the returned
> index value with LB_ERR which is defined as (-1).
>
> Fix:
>
> Extract the index from LOWORD into a WORD variable and
> verify only if the index is smaller than the list size.
> (webrev.02)
>
> This fixes both JDK-8198000 & below mentioned Issue 1.
>
> 2 other Issues:
>
> Issue 1: NON selection of an item.
>
> 1. Currently the returned LONG value is used without
> extracting the LOWORD.
> 2. As far as the HIWORD is 0, the LONG return value would be
> same as index of item(LOWORD).
> 3. But when HIWORD is 1, the LONG return value would be a
> large unexpected value. If it is used as an index, then it
> would result in NON selection of the item.
> 4. Test program:
> http://cr.openjdk.java.net/~arapte/8198000/ListSize3.java
> <http://cr.openjdk.java.net/%7Earapte/8198000/ListSize3.java>
> 5. Steps:
>
> 1. Compile and execute the test program with release
> build JDK.
> 2. Click in the list's client area below the last item,
> i.e. do not click on any item.
>
> 6. Expected behavior: Last item should get selected.
> 7. Actual behavior: Last item does not get selected on first
> click. But the focused item gets selected after few clicks.
>
> Issue 2: Incorrect selection when list size exceeds
> sizeof(WORD) [0xFFFF].
>
> Steps:
>
> 1. Compile and execute the program with release build JDK
> (with or without the fix) :
> http://cr.openjdk.java.net/~arapte/8198000/List65544.java
> <http://cr.openjdk.java.net/%7Earapte/8198000/List65544.java>
> 2. Click the first item in list, press End key.
> 3. Click the last item 65544.
> 4. Expected behavior: The item 65544 should get selected.
> 5. Actual behavior: The item 65544 does not get selected and
> instead item 8 gets selected.
> 6. Verified only windows behavior.
> 7. I suggest to file a new JBS for this issue.
>
> Regards,
>
> Ambarish
>
> *From:*Phil Race
> *Sent:* Thursday, November 01, 2018 1:51 AM
> *To:* Ambarish Rapte <ambarish.rapte at oracle.com>
> <mailto:ambarish.rapte at oracle.com>; awt-dev at openjdk.java.net
> <mailto:awt-dev at openjdk.java.net>
> *Subject:* Re: <AWT Dev> [12] RFR : JDK-8198000 :
> java/awt/List/EmptyListEventTest/EmptyListEventTest.java debug
> assert on Windows
>
> That adds what I suggested, but I had also suggested you leave
> in what you
> had added as it also adds some protection.
>
> Additionally I read the MS docs and they do explain the 131071
> return value.
> The message this code is sending is LB_ITEMFROMPOINT and the
> docs say here
> https://docs.microsoft.com/en-us/windows/desktop/controls/lb-itemfrompoint
> that
>
> >The return value contains the index of the nearest item in
> the *LOWORD*
> <https://msdn.microsoft.com/library/windows/desktop/ms632659>.
> The *HIWORD*
> <https://msdn.microsoft.com/library/windows/desktop/ms632657>
> is zero if the > specified point is in the client area of the
> list box, or one if it is outside the client area"
>
> You got 131071 which is, in hex 0X1FFFF.
>
> So you got "1" for hi-word, meaning "outside client area" and
> "-1" for loword,
> meaning the index. And a return index of "-1" doubtless means
> an error ..
>
> -phil.
>
>
> On 10/31/18 12:49 PM, Ambarish Rapte wrote:
>
> Hi Phil & Sergey,
>
> This issue was introduced with the fix for JDK-6806217
> <https://bugs.openjdk.java.net/browse/JDK-6806217>, in
> 7b55, which modified AwtList::HandleEvent(), so It was not
> observed with JDK6.
>
> Please review the updated change as discussed offline:
> http://cr.openjdk.java.net/~arapte/8198000/webrev.01/
> <http://cr.openjdk.java.net/%7Earapte/8198000/webrev.01/>
>
> Regards,
>
> Ambarish
>
> *From:*Phil Race
> *Sent:* Tuesday, October 30, 2018 2:09 AM
> *To:* Ambarish Rapte <ambarish.rapte at oracle.com>
> <mailto:ambarish.rapte at oracle.com>;
> awt-dev at openjdk.java.net <mailto:awt-dev at openjdk.java.net>
> *Subject:* Re: <AWT Dev> [12] RFR : JDK-8198000 :
> java/awt/List/EmptyListEventTest/EmptyListEventTest.java
> debug assert on Windows
>
> On 10/29/18 7:31 AM, Ambarish Rapte wrote:
>
> Hi,
>
> Please review this Windows only fix,
>
> JBS:
> https://bugs.openjdk.java.net/browse/JDK-8198000
>
> Webrev:
> http://cr.openjdk.java.net/~arapte/8198000/webrev.00/
> <http://cr.openjdk.java.net/%7Earapte/8198000/webrev.00/>
>
> Issue:
>
> 1. Test asserts with debug build jdk, only on windows.
>
>
> Only the debug build turns on asserts.
> But I think JDK 6 always turned on asserts, and this test
> was introduced in 6,
> so something must have changed else we'd have seen this
> test fail a long time ago.
> Can you identify what it was ?
>
> Also same comment as the other bug - you need to add the
> bug id to the test.
>
>
>
>
> 2.
> 3. Assert at Line no 77, awt_List.h :: IsItemSelected()
> 4. awt_List.cpp :: AwtList::HandleEvent() calls
> IsItemSelected() with an incorrect index value.
>
>
> Why ?
>
>
>
>
> 5.
> 6. In AwtList::HandleEvent() , the call
> SendListMessage(LB_ITEMFROMPOINT, 0, msg->lParam)
> returns an arbitrary value 131071, which gets
> passed to IsItemSelected().
> 7. Could not find any relevance to the value 131071,
> from LB_ITEMFROMPOINT doc.
>
>
> That is (128*1024)-1, so it is probably not "arbitrary".
>
> Please add the eval above to the bug report .. once we
> have a complete understanding.
>
> -phil.
>
>
>
>
> 8.
>
> Fix: Index should be verified before making call to
> IsItemSelected() :
> http://cr.openjdk.java.net/~arapte/8198000/webrev.00/
> <http://cr.openjdk.java.net/%7Earapte/8198000/webrev.00/>
>
> Verification: All list tests pass.
>
> Regards,
>
> Ambarish
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/awt-dev/attachments/20181108/f8aa4572/attachment-0001.html>
More information about the awt-dev
mailing list