[RFC][plugin]: fix concurrency problems in PAV.java
Deepak Bhole
dbhole at redhat.com
Tue Apr 12 13:20:55 PDT 2011
* Dr Andrew John Hughes <ahughes at redhat.com> [2011-04-12 16:09]:
> On 15:23 Tue 12 Apr , Denis Lila wrote:
>
> snip...
>
> > diff -r 934543b8084d ChangeLog
> > --- a/ChangeLog Mon Apr 11 16:03:44 2011 +0100
> > +++ b/ChangeLog Tue Apr 12 11:18:57 2011 -0400
> > @@ -1,3 +1,13 @@
> > +2011-04-12 Denis Lila <dlila at redhat.com>
> > +
> > + * plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
> > + (applets, status): Make concurrent.
> > + (PluginAppletViewer): Synchronize appletPanels addElement.
> > + (destroyApplet): Remove applets.containsKey because it and the
> > + get that followed it were not atomic.
> > + (appletPanels): Privatize.
> > + (getApplet, getApplets): Synchronize iteration.
> > +
> > 2011-04-08 Omair Majid <omajid at redhat.com>
> >
> > * README: Update to add notes on rhino and junit.
> > diff -r 934543b8084d plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
> > --- a/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Mon Apr 11 16:03:44 2011 +0100
> > +++ b/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Tue Apr 12 11:18:57 2011 -0400
> > @@ -103,6 +103,8 @@
> > import java.util.List;
> > import java.util.Map;
> > import java.util.Vector;
> > +import java.util.concurrent.ConcurrentHashMap;
> > +import java.util.concurrent.ConcurrentMap;
> >
> > import javax.swing.SwingUtilities;
> >
> > @@ -340,15 +342,15 @@
> > new HashMap<Integer, PluginParseRequest>();
> >
> > // Instance identifier -> PluginAppletViewer object.
> > - private static HashMap<Integer, PluginAppletViewer> applets =
> > - new HashMap<Integer, PluginAppletViewer>();
> > + private static ConcurrentMap<Integer, PluginAppletViewer> applets =
> > + new ConcurrentHashMap<Integer, PluginAppletViewer>();
> >
>
> I don't think this is enough. There are a number of cases where the applets
> collection is misused.
>
> 1.
> while (!applets.containsKey(identifier) && // Map is populated only by reFrame
> (wait < maxWait)) {
>
> try {
> Thread.sleep(50);
> wait += 50;
> } catch (InterruptedException ie) {
> // just wait
> }
> }
>
> Surely this should be a timed wait and additions to applets should be accompanied by
> a notifyAll?
>
> 2.
>
> 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
> // read/writes to files
> waitForAppletInit((NetxPanel) applets.get(identifier).panel);
>
> The two get calls could retrieve different values if a put occurs in the interim.
>
> 3.
>
> // Wait till initialization finishes
> while (!applets.containsKey(identifier) &&
> (
> !status.containsKey(identifier) ||
> status.get(identifier).equals(PAV_INIT_STATUS.PRE_INIT)
> ))
> ;
>
> Again, this busy loop would be better waiting on a notification that the applet has
> been initialised.
>
> 4.
>
> // Wait till initialization finishes
> while (!applets.containsKey(identifier) &&
> (
> !status.containsKey(identifier) ||
> status.get(identifier).equals(PAV_INIT_STATUS.PRE_INIT)
> ))
> ;
>
> // don't bother processing further for inactive applets
> if (status.get(identifier).equals(PAV_INIT_STATUS.INACTIVE))
> return;
>
> applets.get(identifier).handleMessage(reference, message);
>
> Why the multiple get calls? There is the potential for these to return different
> values.
>
> 5.
>
> final int fIdentifier = identifier;
> SwingUtilities.invokeLater(new Runnable() {
> public void run() {
> applets.get(fIdentifier).appletClose();
> }
> });
>
> fIndentifier could point to a different applet. Why not just retain a reference to the applet?
>
>
> > private static PluginStreamHandler streamhandler;
> >
> > private static PluginCallRequestFactory requestFactory;
> >
> > - private static HashMap<Integer, PAV_INIT_STATUS> status =
> > - new HashMap<Integer, PAV_INIT_STATUS>();
> > + private static ConcurrentMap<Integer, PAV_INIT_STATUS> status =
> > + new ConcurrentHashMap<Integer, PAV_INIT_STATUS>();
> >
>
> Again, private static synchronized PAV_INIT_STATUS updateStatus(int identifier, PAV_INIT_STATUS newStatus) {
> calls .get a lot. And
>
> while (!status.get(identifier).equals(PAV_INIT_STATUS.INIT_COMPLETE) && wait < maxWait) {
> try {
> Thread.sleep(50);
> wait += 50;
> } catch (InterruptedException ie) {
> // just wait
> }
> }
>
> Again, shouldn't this be a timed wait?
>
> Is it correct that it could potentially be testing different applets?
>
> > private long handle = 0;
> > private WindowListener windowEventListener = null;
> > @@ -401,8 +403,10 @@
> > this.identifier = identifier;
> > this.panel = appletPanel;
> >
> > - if (!appletPanels.contains(panel))
> > - appletPanels.addElement(panel);
> > + synchronized(appletPanels) {
> > + if (!appletPanels.contains(panel))
> > + appletPanels.addElement(panel);
> > + }
>
> What is the relative rate of mutations (adding new panels, removing dead ones)
> like compared to iteration? If we iterate a lot more than we modify the collection,
> I think a CopyOnWriteArrayList should be considered. Then addIfAbsent can be used here.
>
> >
> > windowEventListener = new WindowAdapter() {
> >
> > @@ -656,8 +660,9 @@
> > PluginDebug.debug("Attempting to destroy frame ", identifier);
> >
> > // Try to dispose the panel right away
> > - if (applets.containsKey(identifier))
> > - applets.get(identifier).dispose();
> > + PluginAppletViewer pav = applets.get(identifier);
> > + if (pav != null)
> > + pav.dispose();
>
> I agree with this change.
>
> I'm worried about the line below:
>
> // If panel is already disposed, return
> if (applets.get(identifier).panel.applet == null) {
> PluginDebug.debug(identifier, " panel inactive. Returning.");
> return;
> }
>
> Shouldn't pav be reussed there too?
> Shouldn't that check be before the dispose call?
>
> >
> > // If panel is already disposed, return
> > if (applets.get(identifier).panel.applet == null) {
> > @@ -895,7 +900,7 @@
> > imageRefs.clear();
> > }
> >
> > - static Vector<AppletPanel> appletPanels = new Vector<AppletPanel>();
> > + private static Vector<AppletPanel> appletPanels = new Vector<AppletPanel>();
> >
> > /**
> > * Get an applet by name.
> > @@ -904,20 +909,22 @@
> > name = name.toLowerCase();
> > SocketPermission panelSp =
> > new SocketPermission(panel.getCodeBase().getHost(), "connect");
> > - for (Enumeration e = appletPanels.elements(); e.hasMoreElements();) {
> > - AppletPanel p = (AppletPanel) e.nextElement();
> > - String param = p.getParameter("name");
> > - if (param != null) {
> > - param = param.toLowerCase();
> > - }
> > - if (name.equals(param) &&
> > - p.getDocumentBase().equals(panel.getDocumentBase())) {
> > + synchronized(appletPanels) {
> > + for (Enumeration e = appletPanels.elements(); e.hasMoreElements();) {
> > + AppletPanel p = (AppletPanel) e.nextElement();
> > + String param = p.getParameter("name");
> > + if (param != null) {
> > + param = param.toLowerCase();
> > + }
> > + if (name.equals(param) &&
> > + p.getDocumentBase().equals(panel.getDocumentBase())) {
>
> The cast to AppletPanel is redundant.
>
> >
> > - SocketPermission sp =
> > + SocketPermission sp =
>
> Indentation change. Shouldn't be in this patch.
>
> > new SocketPermission(p.getCodeBase().getHost(), "connect");
> >
> > - if (panelSp.implies(sp)) {
> > - return p.applet;
> > + if (panelSp.implies(sp)) {
> > + return p.applet;
> > + }
>
> This looks like just brackets being added. Shouldn't be in this patch.
>
> > }
> > }
> > }
> > @@ -933,14 +940,16 @@
> > SocketPermission panelSp =
> > new SocketPermission(panel.getCodeBase().getHost(), "connect");
> >
> > - for (Enumeration<AppletPanel> e = appletPanels.elements(); e.hasMoreElements();) {
> > - AppletPanel p = e.nextElement();
> > - if (p.getDocumentBase().equals(panel.getDocumentBase())) {
> > + synchronized(appletPanels) {
> > + for (Enumeration<AppletPanel> e = appletPanels.elements(); e.hasMoreElements();) {
> > + AppletPanel p = e.nextElement();
> > + if (p.getDocumentBase().equals(panel.getDocumentBase())) {
> >
> > - SocketPermission sp =
> > + SocketPermission sp =
> > new SocketPermission(p.getCodeBase().getHost(), "connect");
> > - if (panelSp.implies(sp)) {
> > - v.addElement(p.applet);
> > + if (panelSp.implies(sp)) {
> > + v.addElement(p.applet);
> > + }
> > }
> > }
> > }
>
>
> I may have missed some stuff here but in that case, it should be clearly documented in the class.
> To be frank, this class is a mess.
Amen to that. It has been carried on since the old plugin.
I will be rewriting significant chunks of it with the security rewrite, as
members and access levels will all be re-evaluated.
/me can't wait for this and JNLPClassLoader to be cleaned up.
Cheers,
Deepak
> --
> 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