Re: Patch: embedded window extension

[Eric Blake <ebb9 at byu dot net>]

Your approach looks okay to me; you solved my initial concern about 
correctness.  But I still have 2 comments.

First, I am not sure whether setAccessible(true) is visible by anyone else 
using reflection; should you be caching the pre-existing accessible value on 
entry and restoring it before existing the priveleged action?  This is more of 
a general question I have about reflection - is getDeclaredField() allowed to 
return the same Field instance to more than one caller, and if so, is there a 
race condition where when one thread has set the Field as accessible inside a 
privileged action, another thread without security privileges can use that 
same Field with the temporary elevation in accessibility even though that 
thread can't change the accessibility?  Or is getDeclaredField() required to 
return a different instance to every caller?  On the other hand, there are 
also security checks for getDeclaredField, so a security manager can enforce 
that only privileged actions can access the Field instance in the first place, 
allowing getDeclaredField to return the same instance to all allowed callers.

Second, how often is this method called?  If it is called frequently, I would 
consider creating a cached version of the anonymous PrivilegedAction, rather 
than creating and destroying one for every invocation.  If it is rarely 
called, then caching is probably not worth the effort, and your code is fine 
as is.

Michael Koch wrote:
> Am Samstag, 17. Juli 2004 17:25 schrieb Eric Blake:
>
> > This patch seems a little too simple for me.  By default,
> > reflection is supposed to follow the same accessibility rules the
> > compiler would follow, meaning that the reflection should throw an
> > IllegalAccessException unless you call setAccessible() on peerField
> > to change the default.  And calling setAccessible requires a
> > security check, so you will have to set up a PrivelegedAction so
> > that your call will always succeed.  Did you just expose a bug in
> > the reflection implementation?
>
>
> Hmm, I used jamvm. Perhaps its not the best candidate to test this 
> code.
>
> I wrote now the attached patch for this problem. Can you please review 
> it ? It makes jamvm hang but I'm not sure where the problem is 
> currently.
>
>
> Michael
>
>
> ------------------------------------------------------------------------
>
> Index: gnu/java/awt/EmbeddedWindow.java
> ===================================================================
> RCS file: /cvsroot/classpath/classpath/gnu/java/awt/EmbeddedWindow.java,v
> retrieving revision 1.3
> diff -u -b -B -r1.3 EmbeddedWindow.java
> --- gnu/java/awt/EmbeddedWindow.java	17 Jul 2004 07:17:04 -0000	1.3
> +++ gnu/java/awt/EmbeddedWindow.java	22 Jul 2004 09:03:37 -0000
> @@ -45,6 +45,8 @@
>  import java.awt.Frame;
>  import java.awt.Toolkit;
>  import java.lang.reflect.Field;
> +import java.security.AccessController;
> +import java.security.PrivilegedAction;
>  
>  /**
>   * Represents an AWT window that can be embedded into another
> @@ -88,14 +90,23 @@
>  
>      if (! (tk instanceof EmbeddedWindowSupport))
>        throw new UnsupportedOperationException
> -        ("Embedded windows are not supported by the current peers: " + tk.getClass());
> +        ("Embedded windows are not supported by the current peers: "
> +	 + tk.getClass());
> +
> +    final EmbeddedWindowSupport support = (EmbeddedWindowSupport) tk;
>  
>      // Circumvent the package-privateness of the AWT internal
>      // java.awt.Component.peer member variable.
> +    AccessController.doPrivileged(new PrivilegedAction()
> +      {
> +	public Object run()
> +	{
>      try
>        {
>  	Field peerField = Component.class.getDeclaredField("peer");
> -	peerField.set(this, ((EmbeddedWindowSupport) tk).createEmbeddedWindow (this));
> +	      peerField.setAccessible(true);
> +	      peerField.set(EmbeddedWindow.this,
> +			    support.createEmbeddedWindow(EmbeddedWindow.this));
>        }
>      catch (IllegalAccessException e)
>        {
> @@ -106,6 +117,10 @@
>  	// This should never happen.
>        }
>  
> +	  return null;
> +        }
> +      });
> +
>      super.addNotify();
>    }
>  

--
Someday, I might put a cute statement here.

Eric Blake             ebb9@byu.net