[RFC][plugin]: fix concurrency problems in PAV.java

[Dr Andrew John Hughes]

On 16:49 Tue 12 Apr     , Denis Lila wrote:
> > 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?
> 
> That's what I thought when I first saw this code. But this works, and I
> see no practical reason to change it. If you really think we should, I
> could do it, but it should be it's own changeset.
> 

I'm not saying it should be in the same changeset.  I was enumerating problems
that still exist with these collections to counter your claim that switching
to CHMs will suddenly fix everything.  It won't.

The practical difference is this code will wake the thread every fifty milliseconds
and reading from the map.  Using notifyAll would only wake waiting threads when the
map has changed.

> > 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.
> 
> Yes, but it doesn't matter. oldFrame is dead code.
> 

Then it needs ripping out so it doesn't confuse people.

> > The cast to AppletPanel is redundant.
> 
> The enumeration is not using generics, so we need the cast.
> 

Ok, so the enumeration needs fixing too.

Is your other patch handling all this generification?  I thought we'd caught most
of them.

> 
> > Indentation change. Shouldn't be in this patch.
> ...<snip>...
> > This looks like just brackets being added. Shouldn't be in this patch.
> 
> The indentation changed because the code was wrapped in
> a synchronized block. Not putting it in this patch would be
> breaking style conventions.
> 

Ok.

Please don't remove code from the mail which is being discussed.  It was
difficult to verify this again here because you removed the code.

> > To be frank, this class is a mess.
> 
> I couldn't agree more.
> And it's not just this class. It's almost every class in the
> icedtea-web sun.applet package.
> 
> As for the points you made that I didn't reply to:
> None of them were pointing out any mistakes in my patch,

No, they were pointing out mistakes in your assumptions that
a switch to CHMs is a fix-all.  It's a start, that's all, but
I'd prefer everything was fixed.

> so can I push this patch, 

Yes.

> and address some of them
> (in particular the comments on destroyApplet) in my second
> patch (the refactoring one)?
> 

Don't mix them up with other changes.  Separate patches please.

> Thank you,
> Denis.
> 
> ----- Original Message -----
> > On 15:23 Tue 12 Apr , Denis Lila wrote:
> > 
> > snip...
> > 
> >
> snip...

-- 
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