<AWT Dev> Review Request For 8146230: Crash in JNI_ArgumentPusherVaArg::JNI_ArgumentPusherVaArg(_jmethodID*, __va_list_tag*)+0xa

[Semyon Sadetsky]

Hi Ambarish,

  Thank you for removing the lock.

But I still do not understand the root cause. waitForEvents() is only 
called on the toolkit thread How it could be executed concurrently?

Are you able to reproduce the issue? Are you sure that correcting method 
IDs initialization really fixes it?

--Semyon


On 7/8/2016 4:24 PM, Ambarish Rapte wrote:
>
> Hi Semyon,
>
>                 Thanks for the review.
>
>                 Please review the webrev.01, updated as per the review 
> comments.
>
> http://cr.openjdk.java.net/~arapte/8146230/webrev.01/ 
> <http://cr.openjdk.java.net/%7Earapte/8146230/webrev.01/>
>
> Changes:
>
>                 Initializing the thread class references in 
> Java_sun_awt_X11_XToolkit_initIDs()
>
>                 As you guided,
>
> 1.LOCK on toolkit thread will be avoided by this change.
>
> 2.awtJNI_ThreadYield() is called for even when just a Frame object is 
> created, which is a case of almost all basic programs.
> Hence it can be early initialized.
>
>                 But regarding, XToolkit.c::get_xawt_root_shell() function.
>
>                 Verified with few sample programs with Frame, this 
> function is not always called.
>
>                 Hence it can be kept as delayed initialization, when 
> required.
>
> Regards,
>
> Ambarish
>
> *From:*Semyon Sadetsky
> *Sent:* Thursday, July 07, 2016 7:27 PM
> *To:* Ambarish Rapte; Sergey Bylokhov; Alexander Scherbatiy; Prasanta 
> Sadhukhan; awt-dev at openjdk.java.net
> *Subject:* Re: Review Request For 8146230: Crash in 
> JNI_ArgumentPusherVaArg::JNI_ArgumentPusherVaArg(_jmethodID*, 
> __va_list_tag*)+0xa
>
> On 07.07.2016 16:14, Ambarish Rapte wrote:
>
>     Hi Semyon,
>
>     1. The _initIDs() functions are called from static initialization
>     block of particular class, hence these functions would be called
>     when class is getting loaded.
>
>          The_initIDs() functions are used to initialize the IDs & get
>     value of Java class members only.
>
>                                     For example:
>
>     Java_sun_awt_X11_XToolkit_initIDs() initializes IDs and gets value
>     from java side XToolkit class (XToolkit.java)
>
> awtJNI_ThreadYield() is only used by the XToolkit. I don't see 
> anything bad if it will be initialized there.
>
>                 2.  But awtJNI_ThreadYield()  is late initialization 
> of IDs. IDs are initialized when first thread has to be yield.
>
> Is lazy initialization really necessary? It looks like the method ID 
> is used always.
>
> ð Addition of operations to Java_sun_awt_X11_XToolkit_initIDs() is 
> addition to class load time.
>
> Lock also introduces delay. Also it maybe a source for deadlocks. 
> Locking toolkit thread using global lock object should be really 
> justified.
>
> ð XToolkit.c::get_xawt_root_shell() is another example of late 
> initialization of jclass, jmethodID.
>
> Should it be fixed as well?
>
> --Semyon
>
> Regards,
>
> Ambarish
>
> *From:*Semyon Sadetsky
> *Sent:* Wednesday, July 06, 2016 11:39 PM
> *To:* Ambarish Rapte; Sergey Bylokhov; Alexander Scherbatiy; Prasanta 
> Sadhukhan; awt-dev at openjdk.java.net <mailto:awt-dev at openjdk.java.net>
> *Subject:* Re: Review Request For 8146230: Crash in 
> JNI_ArgumentPusherVaArg::JNI_ArgumentPusherVaArg(_jmethodID*, 
> __va_list_tag*)+0xa
>
> Hi Ambarish,
>
> Why not simply initialize yieldMethodID separately? For example in 
> Java_sun_awt_X11_XToolkit_initIDs.
>
> --Semyon
>
> On 06.07.2016 14:23, Ambarish Rapte wrote:
>
>     Hi,
>
>                     Please review the fix for JDK9,
>
>                     Bug: https://bugs.openjdk.java.net/browse/JDK-8146230
>
>                     Webrev:
>     http://cr.openjdk.java.net/~arapte/8146230/webrev.00/
>     <http://cr.openjdk.java.net/%7Earapte/8146230/webrev.00/>
>
>     Issue:
>
>     1.Null pointer exception in JNI
>
>     Cause:
>
>     The code block was not multi thread safe.
>
>     Issue occurs in  multi threaded , multi processor environment.
>
>     Fix:
>
>     1.Changed the variable used for double checking, to use
>     /yieldMethodID ./
>
>     2.Changed /yieldMethodID  to  volatile./
>
>     3.Added AWT_LOCK() over initialize block of code.
>
>     4.Removed unrequired  err variable.
>
>     A drawback of Double Check Locking ( DCL ) is, if the resource
>     assignment is not an atomic operation, The DCL may fail.
>
>     But here in the code of concern, is an atomic operation. Hence DCL
>     should work fine.
>
>     Please check below reference link for more detailed discussion of
>     DCL with C++.
>
>     Verification:
>
>     1.Tested Event tests which pass without any regression of this change.
>
>     2.As this change only corrects existing logic, there should be no
>     side effects.
>
>     Reference:
>
>                     C++ and the Perils of Double-Checked Locking:
>     http://www.aristeia.com/Papers/DDJ_Jul_Aug_2004_revised.pdf
>
>     Regards,
>
>     Ambarish
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/awt-dev/attachments/20160708/ddbd99ff/attachment-0001.html>