<AWT Dev> [12] RFR : JDK-8198000 : java/awt/List/EmptyListEventTest/EmptyListEventTest.java debug assert on Windows
Philip Race
philip.race at oracle.com
Sat Nov 3 17:28:46 UTC 2018
- 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>; 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/20181103/0a9660ba/attachment-0001.html>
More information about the awt-dev
mailing list