[RFC][plugin]: fix concurrency problems in PAV.java
Dr Andrew John Hughes
ahughes at redhat.com
Tue Apr 12 16:10:12 PDT 2011
On 17:40 Tue 12 Apr , Deepak Bhole wrote:
> * Denis Lila <dlila at redhat.com> [2011-04-12 17:24]:
> > > Andrew posted some valid concerns in another thread. After those are
> > > all resolved, OK from me.
> >
> > I replied to that e-mail. Some of those were invalid
> > and I pointed those out. The others I think were good
> > points but they weren't directly related to this patch,
> > so I think they should be fixed in a future changeset.
> >
> > The changes to destroyApplet are directly related to
> > this, but I would rather put them in the attached
> > refactoring patch because there's nothing wrong with
> > any of the changes in this patch and if I put them in this
> > patch, I'll have to remake the refactoring patch and
> > that will not be an efficient use of time.
> >
>
> Did you change any functionality in destroyApplet or is it just
> spacing/style fixes?
>
As Deepak is infering here; please don't mix those two in one patch.
If a patch akin to http://hg.openjdk.java.net/jdk6/jdk6/langtools/rev/5c2858bccb3f
goes in, I will not be responsible for my actions.
> Deepak
>
> > Thank you,
> > Denis.
> >
> > ----- Original Message -----
> > > * Denis Lila <dlila at redhat.com> [2011-04-12 16:25]:
> > > > > Perhaps I should have clarified a it more -- I couldn't come up
> > > > > with
> > > > > negative consequences in *our* case as we use integer keys which
> > > > > hash
> > > > > to
> > > > > themselves, so there would never be any lower level buckets to
> > > > > worry
> > > > > about.
> > > > >
> > > > > So essentially we are using HashMap as an indexed list where an
> > > > > index
> > > > > will have only one element and it is never clobbered.
> > > >
> > > > It's true that all the hashes will be different, but just
> > > > because two keys have different hashes does not mean that
> > > > they will go in different chains, so the example I gave
> > > > is still possible, even in our case.
> > > >
> > > > > That said, your point about making an assumption about the
> > > > > internal
> > > > > implementation is a valid concern. Though we are safe now, there
> > > > > is no
> > > > > guarantee of it in the future.
> > > >
> > > > Nice! So, can I push it?
> > > >
> > >
> >
> > >
> > > Cheers,
> > > Deepak
> > >
> > > > ----- Original Message -----
> > > > > * Denis Lila <dlila at redhat.com> [2011-04-12 14:54]:
> > > > > > > You have discarded the check for panel == null. Why?
> > > > > >
> > > > > > Because it's a local variable. If it's null, that's an
> > > > > > infinite loop. Also, if it's null, it can't get
> > > > > > there anyway, because an NPE would be thrown a few lines
> > > > > > prior.
> > > > > >
> > > > >
> > > > > Ah okay, seems like that code was copied and pasted from
> > > > > elsewhere.
> > > > > The
> > > > > panel == null check occurs in multiple places.
> > > > >
> > > > > > > It is unsafe to read only if we iterate over it, which we
> > > > > > > don't.
> > > > > > > ConcurrentHapMap does not lock when retrieving either. From
> > > > > > > the
> > > > > > > JavaDocs:
> > > > > > >
> > > > > > > "However, even though all operations are thread-safe,
> > > > > > > retrieval
> > > > > > > operations do not entail locking ..."
> > > > > > >
> > > > > > > So status is ok.
> > > > > >
> > > > > > > As mentioned above, status is OK as a HashMap. I think rather
> > > > > > > than
> > > > > > > making applets a ConcurrentMap, write to it should be
> > > > > > > synchronized
> > > > > > > instead. The map is written to only in one location, and it
> > > > > > > happens
> > > > > > > only
> > > > > > > once per applet init, so race for it will be rare. OTOH, we
> > > > > > > read
> > > > > > > it in
> > > > > > > many places, and since ConcurrentHashMap does not guarantee
> > > > > > > non-blocking
> > > > > > > read in perpetuity, I think we should avoid it.
> > > > > >
> > > > > > I don't think status is ok, and I don't think synchronizing only
> > > > > > the
> > > > > > writes to applets is right. The docs for HashMap say "If
> > > > > > multiple
> > > > > > threads access a hash map concurrently, and at least one of the
> > > > > > threads modifies the map structurally, it must be synchronized
> > > > > > externally". We satisfy these requirements so we must use a
> > > > > > ConcurrentHashMap (or do our own locking). The reason for this
> > > > > > is
> > > > > > that HashMap was not build with concurrent access in mind and
> > > > > > it's
> > > > > > not safe for "get" to be traversing the internal data structures
> > > > > > of the map while some other thread is modifying these data
> > > > > > structures.
> > > > > >
> > > > > > For example:
> > > > > > suppose a map is implemented by chaining, and suppose we have a
> > > > > > concurrent get(x) and remove(y) in progress. Suppose also that
> > > > > > x.hashCode()==y.hashCode(), so they're in the same chain, and
> > > > > > that
> > > > > > x is after y in the chain. y is removed from the chain with a
> > > > > > sequence of operations like this:
> > > > > > p = y.prev
> > > > > > n = y.next
> > > > > > p.next = n
> > > > > > n.prev = p
> > > > > > y.prev = null
> > > > > > y.next = null
> > > > > >
> > > > > > Now, the chain traversal in get(x) could get to p when p.next ==
> > > > > > y,
> > > > > > so then it goes to y, but then y.next=null could execute, and
> > > > > > get(x)
> > > > > > will think that the end of the chain has been reached and it
> > > > > > will
> > > > > > wrongly return null.
> > > > > >
> > > > >
> > > >
> > > > >
> > > >
> > > > >
> > > > > >
> > > >
> > > > >
> > > > > Deepak
>
> > diff -r 7f589f802faf ChangeLog
> > --- a/ChangeLog Tue Apr 12 11:27:53 2011 -0400
> > +++ b/ChangeLog Tue Apr 12 12:14:32 2011 -0400
> > @@ -1,3 +1,17 @@
> > +2011-04-12 Denis Lila <dlila at redhat.com>
> > +
> > + * plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
> > + Remove unused imports, added various SuppressWarnings annotations.
> > + (createPanel): Return NetxPanel from doPriviledged. Remove dead code.
> > + (PluginParseRequest): Remove - unused.
> > + (defaultSaveFile, label, statusMsgStream, requests, handle): Remove - unused.
> > + (identifier, appletPanels): Privatize.
> > + (applets, status): Use ConcurrentHashMaps.
> > + (framePanel, PluginAppletViewer): Remove unused PrintStream argument.
> > + (forceredraw): Remove - unused.
> > + (getApplets): Use generics.
> > + (appletClose): Fix style to match our convention.
> > +
> > 2011-04-12 Denis Lila <dlila at redhat.com>
> >
> > * plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
> > diff -r 7f589f802faf plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
> > --- a/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Tue Apr 12 11:27:53 2011 -0400
> > +++ b/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Tue Apr 12 12:14:32 2011 -0400
> > @@ -70,23 +70,19 @@
> > import java.awt.Graphics;
> > import java.awt.Image;
> > import java.awt.Insets;
> > -import java.awt.Label;
> > import java.awt.Toolkit;
> > import java.awt.event.WindowAdapter;
> > import java.awt.event.WindowEvent;
> > import java.awt.event.WindowListener;
> > import java.awt.print.PageFormat;
> > import java.awt.print.Printable;
> > -import java.io.BufferedReader;
> > import java.io.IOException;
> > import java.io.InputStream;
> > -import java.io.InputStreamReader;
> > import java.io.PrintStream;
> > import java.io.Reader;
> > import java.io.StringReader;
> > import java.io.UnsupportedEncodingException;
> > import java.lang.reflect.InvocationTargetException;
> > -import java.net.MalformedURLException;
> > import java.net.SocketPermission;
> > import java.net.URI;
> > import java.net.URL;
> > @@ -128,49 +124,27 @@
> > long handle, int x, int y,
> > final URL doc,
> > final Hashtable<String, String> atts) {
> > - AppletViewerPanel panel = AccessController.doPrivileged(new PrivilegedAction<AppletViewerPanel>() {
> > - public AppletViewerPanel run() {
> > - try {
> > - AppletViewerPanel panel = new NetxPanel(doc, atts, false);
> > - AppletViewerPanel.debug("Using NetX panel");
> > - PluginDebug.debug(atts.toString());
> > - return panel;
> > - } catch (Exception ex) {
> > - AppletViewerPanel.debug("Unable to start NetX applet - defaulting to Sun applet", ex);
> > - return new AppletViewerPanel(doc, atts);
> > - }
> > + NetxPanel panel = AccessController.doPrivileged(new PrivilegedAction<NetxPanel>() {
> > + public NetxPanel run() {
> > + NetxPanel panel = new NetxPanel(doc, atts, false);
> > + NetxPanel.debug("Using NetX panel");
> > + PluginDebug.debug(atts.toString());
> > + return panel;
> > }
> > });
> >
> > // create the frame.
> > - PluginAppletViewer.framePanel(identifier, System.out, handle, panel);
> > + PluginAppletViewer.framePanel(identifier, handle, panel);
> >
> > panel.init();
> >
> > // Start the applet
> > initEventQueue(panel);
> >
> > - // Applet initialized. Find out it's classloader and add it to the list
> > - String portComponent = doc.getPort() != -1 ? ":" + doc.getPort() : "";
> > - String codeBase = doc.getProtocol() + "://" + doc.getHost() + portComponent;
> > -
> > - if (atts.get("codebase") != null) {
> > - try {
> > - URL appletSrcURL = new URL(codeBase + atts.get("codebase"));
> > - codeBase = appletSrcURL.getProtocol() + "://" + appletSrcURL.getHost();
> > - } catch (MalformedURLException mfue) {
> > - // do nothing
> > - }
> > - }
> > -
> > - // Wait for the panel to initialize
> > - // (happens in a separate thread)
> > - Applet a;
> > -
> > // Wait for panel to come alive
> > int maxWait = PluginAppletViewer.APPLET_TIMEOUT; // wait for panel to come alive
> > int wait = 0;
> > - while ((panel == null) || (!((NetxPanel) panel).isAlive() && wait < maxWait)) {
> > + while (!panel.isAlive() && wait < maxWait) {
> > try {
> > Thread.sleep(50);
> > wait += 50;
> > @@ -183,10 +157,10 @@
> > // (happens in a separate thread)
> > PluginAppletViewer.waitForAppletInit((NetxPanel) panel);
> >
> > - a = panel.getApplet();
> > + Applet a = panel.getApplet();
> >
> > // Still null?
> > - if (panel.getApplet() == null) {
> > + if (a == null) {
> > streamhandler.write("instance " + identifier + " reference " + -1 + " fatalError " + "Initialization failed");
> > return null;
> > }
> > @@ -194,8 +168,8 @@
> > PluginDebug.debug("Applet ", a.getClass(), " initialized");
> > streamhandler.write("instance " + identifier + " reference 0 initialized");
> >
> > - AppletSecurityContextManager.getSecurityContext(0).associateSrc(((NetxPanel) panel).getAppletClassLoader(), doc);
> > - AppletSecurityContextManager.getSecurityContext(0).associateInstance(identifier, ((NetxPanel) panel).getAppletClassLoader());
> > + AppletSecurityContextManager.getSecurityContext(0).associateSrc(panel.getAppletClassLoader(), doc);
> > + AppletSecurityContextManager.getSecurityContext(0).associateInstance(identifier, panel.getAppletClassLoader());
> >
> > return panel;
> > }
> > @@ -291,21 +265,12 @@
> > }
> > }
> >
> > -class PluginParseRequest {
> > - long handle;
> > - String tag;
> > - String documentbase;
> > -}
> > -
> > /*
> > */
> > // FIXME: declare JSProxy implementation
> > + at SuppressWarnings("serial")
> > public class PluginAppletViewer extends XEmbeddedFrame
> > implements AppletContext, Printable {
> > - /**
> > - * Some constants...
> > - */
> > - private static String defaultSaveFile = "Applet.ser";
> >
> > /**
> > * Enumerates the current status of an applet
> > @@ -323,23 +288,9 @@
> > /**
> > * The panel in which the applet is being displayed.
> > */
> > - AppletViewerPanel panel;
> > + private AppletViewerPanel panel;
> >
> > - /**
> > - * The status line.
> > - */
> > - Label label;
> > -
> > - /**
> > - * output status messages to this stream
> > - */
> > -
> > - PrintStream statusMsgStream;
> > -
> > - int identifier;
> > -
> > - private static HashMap<Integer, PluginParseRequest> requests =
> > - new HashMap<Integer, PluginParseRequest>();
> > + private int identifier;
> >
> > // Instance identifier -> PluginAppletViewer object.
> > private static ConcurrentMap<Integer, PluginAppletViewer> applets =
> > @@ -352,7 +303,6 @@
> > private static ConcurrentMap<Integer, PAV_INIT_STATUS> status =
> > new ConcurrentHashMap<Integer, PAV_INIT_STATUS>();
> >
> > - private long handle = 0;
> > private WindowListener windowEventListener = null;
> > private AppletEventListener appletEventListener = null;
> >
> > @@ -370,15 +320,14 @@
> > public PluginAppletViewer() {
> > }
> >
> > - public static void framePanel(int identifier, PrintStream statusMsgStream,
> > - long handle, AppletViewerPanel panel) {
> > + public static void framePanel(int identifier, long handle, NetxPanel panel) {
> >
> > PluginDebug.debug("Framing ", panel);
> >
> > // SecurityManager MUST be set, and only privileged code may call reFrame()
> > System.getSecurityManager().checkPermission(new AllPermission());
> >
> > - PluginAppletViewer appletFrame = new PluginAppletViewer(handle, identifier, statusMsgStream, panel);
> > + PluginAppletViewer appletFrame = new PluginAppletViewer(handle, identifier, panel);
> >
> > appletFrame.add("Center", panel);
> > appletFrame.pack();
> > @@ -395,11 +344,9 @@
> > * Create new plugin appletviewer frame
> > */
> > private PluginAppletViewer(long handle, final int identifier,
> > - PrintStream statusMsgStream,
> > - AppletViewerPanel appletPanel) {
> > + NetxPanel appletPanel) {
> >
> > super(handle, true);
> > - this.statusMsgStream = statusMsgStream;
> > this.identifier = identifier;
> > this.panel = appletPanel;
> >
> > @@ -547,8 +494,6 @@
> > if (wait >= maxWait)
> > throw new Exception("Applet initialization timeout");
> >
> > - PluginAppletViewer oldFrame = applets.get(identifier);
> > -
> > // We should not try to destroy an applet during
> > // initialization. It may cause an inconsistent state,
> > // which would bad if it's a trusted applet that
> > @@ -792,7 +737,7 @@
> >
> > // Still null?
> > if (panel.getApplet() == null) {
> > - this.streamhandler.write("instance " + identifier + " reference " + -1 + " fatalError " + "Initialization failed");
> > + streamhandler.write("instance " + identifier + " reference " + -1 + " fatalError " + "Initialization failed");
> > return;
> > }
> >
> > @@ -807,11 +752,6 @@
> > }
> > }
> >
> > - // FIXME: Kind of hackish way to ensure synchronized re-drawing
> > - private synchronized void forceredraw() {
> > - doLayout();
> > - }
> > -
> > /*
> > * Methods for java.applet.AppletContext
> > */
> > @@ -910,8 +850,8 @@
> > SocketPermission panelSp =
> > new SocketPermission(panel.getCodeBase().getHost(), "connect");
> > synchronized(appletPanels) {
> > - for (Enumeration e = appletPanels.elements(); e.hasMoreElements();) {
> > - AppletPanel p = (AppletPanel) e.nextElement();
> > + for (Enumeration<AppletPanel> e = appletPanels.elements(); e.hasMoreElements();) {
> > + AppletPanel p = e.nextElement();
> > String param = p.getParameter("name");
> > if (param != null) {
> > param = param.toLowerCase();
> > @@ -1652,34 +1592,32 @@
> > //
> > final AppletPanel p = panel;
> >
> > - new Thread(new Runnable()
> > - {
> > - public void run()
> > - {
> > - ClassLoader cl = p.applet.getClass().getClassLoader();
> > -
> > - // Since we want to deal with JNLPClassLoader, extract it if this
> > - // is a codebase loader
> > - if (cl instanceof JNLPClassLoader.CodeBaseClassLoader)
> > - cl = ((JNLPClassLoader.CodeBaseClassLoader) cl).getParentJNLPClassLoader();
> > + new Thread(new Runnable() {
> > + @SuppressWarnings("deprecation")
> > + public void run() {
> > + ClassLoader cl = p.applet.getClass().getClassLoader();
> >
> > - ThreadGroup tg = ((JNLPClassLoader) cl).getApplication().getThreadGroup();
> > + // Since we want to deal with JNLPClassLoader, extract it if this
> > + // is a codebase loader
> > + if (cl instanceof JNLPClassLoader.CodeBaseClassLoader)
> > + cl = ((JNLPClassLoader.CodeBaseClassLoader) cl).getParentJNLPClassLoader();
> >
> > - appletShutdown(p);
> > - appletPanels.removeElement(p);
> > - dispose();
> > + ThreadGroup tg = ((JNLPClassLoader) cl).getApplication().getThreadGroup();
> >
> > - if (tg.activeCount() > 0)
> > - tg.stop();
> > + appletShutdown(p);
> > + appletPanels.removeElement(p);
> > + dispose();
> >
> > - if (countApplets() == 0) {
> > - appletSystemExit();
> > - }
> > + if (tg.activeCount() > 0)
> > + tg.stop();
> >
> > - updateStatus(identifier, PAV_INIT_STATUS.DESTROYED);
> > - }
> > - }).start();
> > + if (countApplets() == 0) {
> > + appletSystemExit();
> > + }
> >
> > + updateStatus(identifier, PAV_INIT_STATUS.DESTROYED);
> > + }
> > + }).start();
> > }
> >
> > /**
> > @@ -1868,6 +1806,7 @@
> > });
> > }
> >
> > + @SuppressWarnings("unused")
> > public static void parse(int identifier, long handle, String width,
> > String height, Reader in, URL url,
> > PrintStream statusMsgStream,
>
--
Andrew :)
Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)
Support Free Java!
Contribute to GNU Classpath and IcedTea
http://www.gnu.org/software/classpath
http://icedtea.classpath.org
PGP Key: F5862A37 (https://keys.indymedia.org/)
Fingerprint = EA30 D855 D50F 90CD F54D 0698 0713 C3ED F586 2A37
More information about the distro-pkg-dev
mailing list