[icedtea-web] RFC: Patch to stabilize applet initialization
Andrew Su
asu at redhat.com
Fri Oct 22 07:35:01 PDT 2010
----- "Deepak Bhole" <dbhole at redhat.com> wrote:
> From: "Deepak Bhole" <dbhole at redhat.com>
> To: "IcedTea Distro List" <distro-pkg-dev at openjdk.java.net>
> Sent: Thursday, October 21, 2010 5:41:11 PM GMT -05:00 US/Canada Eastern
> Subject: Re: [icedtea-web] RFC: Patch to stabilize applet initialization
>
> * Deepak Bhole <dbhole at redhat.com> [2010-10-21 17:38]:
> > Hi,
> >
> > The attached patch envelopes fixes for two of Andrew's pending
> patches
> > (fixing 100% CPU load and the frame pop-up issue). In addition to
> those,
> > it fixes a lot of other initialization issues, particularly when
> > initializing multiple applets and/or closing other tabs with
> applets
> > which are being initialized.
> >
> > With this patch, I tested over 200 parallel applet inits, randomly
> > closing tabs, and everything works correctly.
> >
> > One known issue is that if timed correctly, a frame can appear
> outside
> > for a fraction of a second. It always goes away instantly though,
> and I
> > will be posting a patch to fix that problem in the near future.
> >
> > Cheers,
> > Deepak
>
> Please ignore the spacing issues. I will be normalizing all spaces
> for
> all of icedtea-web in another commit after all pending patches are
> in.
>
> Cheers,
> Deepak
>
> > diff -r 85db7b3a1c93
> plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
> > --- a/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Thu
> Oct 21 21:12:21 2010 +0100
> > +++ b/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Thu
> Oct 21 17:22:56 2010 -0700
> > @@ -136,12 +136,10 @@
> > }
> > }
> > });
> > +
> > + // create the frame.
> > + PluginAppletViewer.reFrame(null, identifier, System.out,
> 0, panel);
> >
> > -
> > -
> > - // create the frame.
> > - PluginAppletViewer.reFrame(null, identifier, System.out,
> handle, panel);
> > -
> > panel.init();
> >
> > // Start the applet
> > @@ -179,15 +177,7 @@
> >
> > // Wait for the panel to initialize
> > // (happens in a separate thread)
> > - while (panel.getApplet() == null &&
> > - ((NetxPanel) panel).isAlive()) {
> > - try {
> > - Thread.sleep(50);
> > - PluginDebug.debug("Waiting for applet to
> initialize...");
> > - } catch (InterruptedException ie) {
> > - // just wait
> > - }
> > - }
> > + PluginAppletViewer.waitForAppletInit((NetxPanel) panel);
> >
> > a = panel.getApplet();
> >
> > @@ -317,7 +307,17 @@
> > */
> > private static String defaultSaveFile = "Applet.ser";
> >
> > - private static enum PAV_INIT_STATUS {PRE_INIT, IN_INIT,
> INIT_COMPLETE, INACTIVE};
> > +
> > + /**
> > + * Enumerates the current status of an applet
> > + *
> > + * PRE_INIT -> Parsing and initialization phase
> > + * INIT_COMPLETE -> Initialization complete, reframe pending
> > + * REFRAME_COMPLETE -> Reframe complete, applet is
> initialized and usable by the user
> > + * INACTIVE -> Browser has directed that the applet be
> destroyed (this state is non-overridable except by DESTROYED)
> > + * DESTROYED -> Applet has been destroyed
> > + */
> > + private static enum PAV_INIT_STATUS {PRE_INIT, INIT_COMPLETE,
> REFRAME_COMPLETE, INACTIVE, DESTROYED};
> >
> > /**
> > * The panel in which the applet is being displayed.
> > @@ -350,7 +350,6 @@
> >
> > private static HashMap<Integer, PAV_INIT_STATUS> status =
> > new HashMap<Integer,PAV_INIT_STATUS>();
> > -
> >
> > private long handle = 0;
> > private WindowListener windowEventListener = null;
> > @@ -380,16 +379,20 @@
> > return;
> >
> > PluginAppletViewer newFrame = new
> PluginAppletViewer(handle, identifier, statusMsgStream, panel);
> > -
> > +
> > if (oldFrame != null) {
> > applets.remove(oldFrame.identifier);
> >
> oldFrame.removeWindowListener(oldFrame.windowEventListener);
> >
> panel.removeAppletListener(oldFrame.appletEventListener);
> > +
> > + // Add first, remove later
> > + newFrame.add("Center", panel);
> > oldFrame.remove(panel);
> > oldFrame.dispose();
> > + } else {
> > + newFrame.add("Center", panel);
> > }
> >
> > - newFrame.add("Center", panel);
> > newFrame.pack();
> >
> > newFrame.appletEventListener = new
> AppletEventListener(newFrame, newFrame);
> > @@ -397,11 +400,6 @@
> >
> > applets.put(identifier, newFrame);
> >
> > - // dispose oldframe if necessary
> > - if (oldFrame != null) {
> > - oldFrame.dispose();
> > - }
> > -
> > PluginDebug.debug(panel + " reframed");
> > }
> >
> > @@ -449,7 +447,7 @@
> > this.frame = frame;
> > this.appletViewer = appletViewer;
> > }
> > -
> > +
> > public void appletStateChanged(AppletEvent evt)
> > {
> > AppletPanel src = (AppletPanel)evt.getSource();
> > @@ -483,9 +481,9 @@
> > AppletPanel.changeFrameAppContext(frame,
> SunToolkit.targetToAppContext(a));
> > else
> > AppletPanel.changeFrameAppContext(frame,
> AppContext.getAppContext());
> > -
> > - status.put(appletViewer.identifier,
> PAV_INIT_STATUS.INIT_COMPLETE);
> > -
> > +
> > + updateStatus(appletViewer.identifier,
> PAV_INIT_STATUS.INIT_COMPLETE);
> > +
> > break;
> > }
> > }
> > @@ -510,7 +508,13 @@
> >
> > try {
> > if (message.startsWith("handle")) {
> > -
> > +
> > + // If there is a key for this status, it means it
> > + // was either initialized before, or destroy has been
>
> > + // processed. Stop moving further.
> > + if (updateStatus(identifier,
> PAV_INIT_STATUS.PRE_INIT) != null)
> > + return;
> > +
> > // Extract the information from the message
> > String[] msgParts = new String[4];
> > for (int i=0; i < 3; i++) {
> > @@ -519,32 +523,38 @@
> > msgParts[i] = message.substring(spaceLocation + 1,
> nextSpaceLocation);
> > message = message.substring(nextSpaceLocation + 1);
> > }
> > -
> > +
> > long handle = Long.parseLong(msgParts[0]);
> > String width = msgParts[1];
> > String height = msgParts[2];
> > -
> > +
> > int spaceLocation = message.indexOf(' ',
> "tag".length()+1);
> > String documentBase =
> >
> UrlUtil.decode(message.substring("tag".length() + 1, spaceLocation));
> > - String tag = message.substring(spaceLocation+1);
> > + String tag = message.substring(spaceLocation+1);
> > +
> > + // Decode the tag
> > + tag = tag.replace(">", ">");
> > + tag = tag.replace("<", "<");
> > + tag = tag.replace("&", "&");
> > + tag = tag.replace(" ", "\n");
> > + tag = tag.replace(" ", "\r");
> > + tag = tag.replace(""", "\"");
> >
> > PluginDebug.debug ("Handle = " + handle + "\n" +
> > "Width = " + width + "\n" +
> > "Height = " + height + "\n" +
> > "DocumentBase = " + documentBase +
> "\n" +
> > "Tag = " + tag);
> > -
> > - status.put(identifier,
> PAV_INIT_STATUS.PRE_INIT);
> > +
> > PluginAppletViewer.parse
> > (identifier, handle, width, height,
> > new StringReader(tag),
> > new URL(documentBase));
> > -
> >
> > int maxWait = APPLET_TIMEOUT; // wait for applet
> to fully load
> > int wait = 0;
> > - while
> (!status.get(identifier).equals(PAV_INIT_STATUS.INIT_COMPLETE) &&
> > + while ( !applets.containsKey(identifier) && // Map
> is populated only by reFrame
> > (wait < maxWait)) {
> >
> > try {
> > @@ -555,9 +565,44 @@
> > }
> > }
> >
> > - if
> (!status.get(identifier).equals(PAV_INIT_STATUS.INIT_COMPLETE))
> > + // If wait exceeded maxWait, we timed out. Throw
> an exception
> > + 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
> > + // read/writes to files
> > + waitForAppletInit((NetxPanel)
> applets.get(identifier).panel);
> > +
> > + // Should we proceed with reframing?
> > + if
> (status.get(identifier).equals(PAV_INIT_STATUS.INACTIVE)) {
> > + destroyApplet(identifier);
> > + return;
> > + }
> > +
> > + // Proceed with re-framing
> > + reFrame(oldFrame, identifier, System.out, handle,
> oldFrame.panel);
> > +
> > + // There is a slight chance that destroy can
> happen
> > + // between the above and below line
> > + if (updateStatus(identifier,
> PAV_INIT_STATUS.REFRAME_COMPLETE).equals(PAV_INIT_STATUS.INACTIVE)) {
> > + destroyApplet(identifier);
> > + }
> > +
> > + } else if (message.startsWith("destroy")) {
> > +
> > + // Set it inactive, and try to do cleanup is
> applicable
> > + PAV_INIT_STATUS previousStatus =
> updateStatus(identifier, PAV_INIT_STATUS.INACTIVE);
> > + PluginDebug.debug("Destroy status set for " +
> identifier);
> > +
> > + if (previousStatus != null &&
> > +
> previousStatus.equals(PAV_INIT_STATUS.REFRAME_COMPLETE)) {
> > + destroyApplet(identifier);
> > + }
> > +
> > } else {
> > PluginDebug.debug ("Handling message: " + message
> + " instance " + identifier + " " + Thread.currentThread());
> >
> > @@ -568,7 +613,7 @@
> >
> 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;
> > @@ -580,31 +625,131 @@
> > e.printStackTrace();
> >
> > // If an exception happened during pre-init, we need
> to update status
> > - if
> (status.get(identifier).equals(PAV_INIT_STATUS.PRE_INIT))
> > - status.put(identifier, PAV_INIT_STATUS.INACTIVE);
> > + updateStatus(identifier, PAV_INIT_STATUS.INACTIVE);
> >
> > throw new RuntimeException("Failed to handle message:
> " +
> > message + " for instance " + identifier, e);
> > }
> > }
> > -
> > +
> > + /**
> > + * Sets the status unless an overriding status is set (e.g. if
>
> > + * status is DESTROYED, it may not be overridden).
> > + *
> > + * @param identifier The identifier for which the status is to
> be set
> > + * @param status The status to switch to
> > + * @return The previous status
> > + */
> > + private static synchronized PAV_INIT_STATUS updateStatus(int
> identifier, PAV_INIT_STATUS newStatus) {
> > +
> > + PAV_INIT_STATUS prev = status.get(identifier);
> > +
> > + // If the status is set
> > + if (status.containsKey(identifier)) {
> > +
> > + // Nothing may override destroyed status
> > + if
> (status.get(identifier).equals(PAV_INIT_STATUS.DESTROYED)) {
> > + return prev;
> > + }
> > +
> > + // If status is inactive, only DESTROYED may override it
> > + if
> (status.get(identifier).equals(PAV_INIT_STATUS.INACTIVE)) {
> > + if (!newStatus.equals(PAV_INIT_STATUS.DESTROYED)) {
> > + return prev;
Status isn't updated to PAV_INIT_STATUS.DESTROYED.
> > + }
> > + }
> > + }
> > +
> > + // Else set to given status
> > + status.put(identifier, newStatus);
> > +
> > + return prev;
> > + }
> > +
> > + /**
> > + * Destroys the given applet instance.
> > + *
> > + * This function may be called multiple times without
> problems.
> > + * It does a synchronized check on the status and will only
> > + * attempt to destroy the applet if not previously destroyed.
> > + *
> > + * @param identifier The instance which is to be destroyed
> > + */
> > +
> > + private static synchronized void destroyApplet(int identifier)
> {
> > +
> > + PluginDebug.debug("DestroyApplet called for " + identifier);
> > +
> > + PAV_INIT_STATUS prev = updateStatus(identifier,
> PAV_INIT_STATUS.DESTROYED);
> > +
> > + // If already destroyed, return
> > + if (prev.equals(PAV_INIT_STATUS.DESTROYED)) {
> > + PluginDebug.debug(identifier + " already destroyed.
> Returning.");
> > + return;
> > + }
> > +
> > + // If already disposed, return
> > + if (applets.get(identifier).panel.applet == null) {
> > + // Try to still dispose the panel itself -- no harm done
> with double dispose
> > + applets.get(identifier).dispose();
> > +
> > + PluginDebug.debug(identifier + " inactive. Returning.");
> > + return;
> > + }
> > +
> > + PluginDebug.debug("Attempting to destroy " + identifier);
> > +
> > + final int fIdentifier = identifier;
> > + SwingUtilities.invokeLater(new Runnable() {
> > + public void run() {
> > + applets.get(fIdentifier).appletClose();
> > + }
> > + });
> > +
> > + PluginDebug.debug(identifier + " destroyed");
> > + }
> > +
> > + /**
> > + * Function to block until applet initialization is complete
> > + *
> > + * @param identifier The instance to wait for
> > + */
> > + public static void waitForAppletInit(NetxPanel panel) {
> > +
> > + int waitTime = 0;
> > +
> > + // Wait till initialization finishes
> > + while (panel.getApplet() == null &&
> > + panel.isAlive() &&
> > + waitTime < APPLET_TIMEOUT) {
> > + try {
> > + if (waitTime%500 == 0)
> > + PluginDebug.debug("Waiting for applet panel " +
> panel + " to initialize...");
> > +
> > + Thread.sleep(waitTime += 50);
> > + } catch (InterruptedException ie) {
> > + // just wait
> > + }
> > + }
> > +
> > + PluginDebug.debug("Applet panel " + panel + "
> initialized");
> > + }
> > +
> > public void handleMessage(int reference, String message)
> > {
> > if (message.startsWith("width")) {
> >
> > // Wait for panel to come alive
> > int maxWait = APPLET_TIMEOUT; // wait for panel to
> come alive
> > - int wait = 0;
> > + int wait = 0;
> > while
> (!status.get(identifier).equals(PAV_INIT_STATUS.INIT_COMPLETE) && wait
> < maxWait) {
> > -
> > - try {
> > - Thread.sleep(50);
> > - wait += 50;
> > - } catch (InterruptedException ie) {
> > - // just wait
> > - }
> > - }
> > -
> > + try {
> > + Thread.sleep(50);
> > + wait += 50;
> > + } catch (InterruptedException ie) {
> > + // just wait
> > + }
> > + }
> >
> > // 0 => width, 1=> width_value, 2 => height, 3=>
> height_value
> > String[] dimMsg = message.split(" ");
> > @@ -636,7 +781,7 @@
> >
> > panel.setSize(width, height);
> > panel.validate();
> > -
> > +
> > panel.applet.resize(width, height);
> > panel.applet.validate();
> > }
> > @@ -649,9 +794,6 @@
> > e.printStackTrace();
> > }
> >
> > - } else if (message.startsWith("destroy")) {
> > - dispose();
> > - status.put(identifier, PAV_INIT_STATUS.INACTIVE);
> > } else if (message.startsWith("GetJavaObject")) {
> >
> > // FIXME: how do we determine what security context
> this
> > @@ -672,15 +814,7 @@
> >
> > // Wait for the panel to initialize
> > // (happens in a separate thread)
> > - while (panel.getApplet() == null &&
> > - ((NetxPanel) panel).isAlive()) {
> > - try {
> > - Thread.sleep(50);
> > - PluginDebug.debug("Waiting for applet to
> initialize...");
> > - } catch (InterruptedException ie) {
> > - // just wait
> > - }
> > - }
> > + waitForAppletInit((NetxPanel) panel);
> >
> > PluginDebug.debug(panel + " -- " + panel.getApplet() +
> " -- " + ((NetxPanel) panel).isAlive());
> >
> > @@ -1548,10 +1682,11 @@
> > if (countApplets() == 0) {
> > appletSystemExit();
> > }
> > +
> > + updateStatus(identifier,
> PAV_INIT_STATUS.DESTROYED);
> > }
> > }).start();
> >
> > - status.put(identifier, PAV_INIT_STATUS.INACTIVE);
> > }
> >
> > /**
> > @@ -1677,18 +1812,6 @@
> > val = buf.toString();
> > }
> >
> > - att = att.replace(">", ">");
> > - att = att.replace("<", "<");
> > - att = att.replace("&", "&");
> > - att = att.replace(" ", "\n");
> > - att = att.replace(" ", "\r");
> > -
> > - val = val.replace(">", ">");
> > - val = val.replace("<", "<");
> > - val = val.replace("&", "&");
> > - val = val.replace(" ", "\n");
> > - val = val.replace(" ", "\r");
> > -
> > PluginDebug.debug("PUT " + att + " = '" + val + "'");
> > atts.put(att.toLowerCase(java.util.Locale.ENGLISH), val);
> >
> > @@ -1708,7 +1831,6 @@
> > // private static final == inline
> > private static final boolean isInt(Object o) {
> > boolean isInt = false;
> > -
> > try {
> > Integer.parseInt((String) o);
> > isInt = true;
> > @@ -1763,7 +1885,6 @@
> > } catch (IOException ioe) {
> > return ioe;
> > }
> > -
> > return null;
> > }
> > };
> > @@ -1785,6 +1906,7 @@
> > boolean isObjectTag = false;
> > boolean isEmbedTag = false;
> > boolean objectTagAlreadyParsed = false;
> > +
> > // The current character
> > // FIXME: This is an evil hack to force
> pass-by-reference.. the
> > // parsing code needs to be rewritten from scratch to
> prevent such
> > @@ -1879,19 +2001,6 @@
> > if (val == null) {
> >
> statusMsgStream.println(requiresNameWarning);
> > } else if (atts != null) {
> > - att = att.replace(">", ">");
> > - att = att.replace("<", "<");
> > - att = att.replace("&", "&");
> > - att = att.replace(" ", "\n");
> > - att = att.replace(" ", "\r");
> > - att = att.replace(""",
> "\"");
> > -
> > - val = val.replace(">", ">");
> > - val = val.replace("<", "<");
> > - val = val.replace("&", "&");
> > - val = val.replace(" ", "\n");
> > - val = val.replace(" ", "\r");
> > - val = val.replace(""",
> "\"");
> > PluginDebug.debug("PUT " + att + "
> = " + val);
> > atts.put(att.toLowerCase(), val);
> > } else {
> > @@ -1920,8 +2029,8 @@
> >
> > if (atts.get("width") == null ||
> !isInt(atts.get("width"))) {
> > atts.put("width", width);
> > - }
> > -
> > + }
> > +
> > if (atts.get("height") == null ||
> !isInt(atts.get("height"))) {
> > atts.put("height", height);
> > }
> > @@ -1974,7 +2083,7 @@
> > if (atts.get("width") == null ||
> !isInt(atts.get("width"))) {
> > atts.put("width", width);
> > }
> > -
> > +
> > if (atts.get("height") == null ||
> !isInt(atts.get("height"))) {
> > atts.put("height", height);
> > }
> > @@ -2023,10 +2132,11 @@
> > if (atts.get("width") == null ||
> !isInt(atts.get("width"))) {
> > atts.put("width", width);
> > }
> > -
> > +
> > if (atts.get("height") == null ||
> !isInt(atts.get("height"))) {
> > atts.put("height", height);
> > }
> > +
> > }
> > }
> > }
> > diff -r 85db7b3a1c93
> plugin/icedteanp/java/sun/applet/PluginMessageConsumer.java
> > --- a/plugin/icedteanp/java/sun/applet/PluginMessageConsumer.java
> Thu Oct 21 21:12:21 2010 +0100
> > +++ b/plugin/icedteanp/java/sun/applet/PluginMessageConsumer.java
> Thu Oct 21 17:22:56 2010 -0700
> > @@ -126,6 +126,10 @@
> > }
> >
> > private String getPriorityStrIfPriority(String message) {
> > +
> > + // Destroy messages are permanently high priority
> > + if (message.endsWith("destroy"))
> > + return "destroy";
> >
> > synchronized (priorityWaitQueue) {
> > Iterator<String> it = priorityWaitQueue.iterator();
> > @@ -150,7 +154,6 @@
> > }
> > }
> >
> > -
> > public void notifyWorkerIsFree(PluginMessageHandlerWorker worker)
> {
> > synchronized (initWorkers) {
> > Iterator<Integer> i = initWorkers.keySet().iterator();
> > @@ -176,6 +179,32 @@
> > }
> >
> > protected class ConsumerThread extends Thread {
> > +
> > + /**
> > + * Scans the readQueue for priority messages and brings them to
> the front
> > + */
> > + private void bringPriorityMessagesToFront() {
> > + synchronized (readQueue) {
> > +
> > + // iterate through the list
> > + for (int i=0; i < readQueue.size(); i++) {
> > +
> > + // remove element at i to inspect it
> > + String message = readQueue.remove(i);
> > +
> > + // if element at i is a priority msg, bring it forward
> > + if (getPriorityStrIfPriority(message) != null) {
> > + readQueue.addFirst(message);
> > + } else { // else keep it where it was
> > + readQueue.add(i, message);
> > + }
> > +
> > + // by the end the queue size is the same, so the
> > + // position indicator (i) is still valid
> > + }
> > + }
> > + }
I believe that splitting it to atleast two queue, one for priority and one for all other messages would be a bit better performance wise.
Since:
-- getPriorityStrIfPriority(): assume constant
-- Adding to proper queue: constant
-- Checking if queue is empty: constant
-- Reading and deleting message: constant.
Right now, doing N checks for each time we try to read a message would be a bit more expensive.
Splitting it up could potentially save us one extra call to getPriorityStrIfPriority() for each message.
Using an extra queue, let's call it PQ, we can store the priorityStr into PQ if we determined it is a priority message, this way we don't need to call getPriorityStrIfPriority() on each message again later.
> > +
> > public void run() {
> >
> > while (true) {
> > @@ -190,7 +219,6 @@
> >
> > String[] msgParts = message.split(" ");
> >
> > -
> > String priorityStr =
> getPriorityStrIfPriority(message);
> > boolean isPriorityResponse = (priorityStr !=
> null);
> >
> > @@ -199,9 +227,12 @@
> >
> > if (worker == null) {
> > synchronized(readQueue) {
> > - readQueue.addLast(message);
> > + readQueue.addFirst(message);
> > }
> >
> > + // re-scan to see if any priority message came
> in
> > + bringPriorityMessagesToFront();
> > +
> > continue; // re-loop to try next msg
> > }
> >
Just the one change to updateStatus.
The extra queue idea gives a minor boost for performance. (This change is optional, and can go in later if needed)
Everything else looks fine. Okay to commit after the change.
Cheers,
Andrew
More information about the distro-pkg-dev
mailing list