[RFC][plugin]: fix concurrency problems in PAV.java
Dr Andrew John Hughes
ahughes at redhat.com
Tue Apr 12 13:09:47 PDT 2011
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.
--
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