[RFC][plugin]: fix concurrency problems in PAV.java
Deepak Bhole
dbhole at redhat.com
Tue Apr 12 14:40:16 PDT 2011
* 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?
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,
More information about the distro-pkg-dev
mailing list