<AWT Dev> [12] RFR : JDK-8198000 : java/awt/List/EmptyListEventTest/EmptyListEventTest.java debug assert on Windows
Philip Race
philip.race at oracle.com
Tue Nov 6 16:50:38 UTC 2018
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>
> *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
>
> - 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/20181106/b529f078/attachment-0001.html>
More information about the awt-dev
mailing list