Mercurial > hg > icedtea7

changeset 1928:ee421053683d

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
- Fix race conditions related to multi-stage initialization/destruction - Account for port# when implementing security policy based on address - Reduce sleep time during initialization check iterations
author Deepak Bhole <dbhole@redhat.com>
date Wed, 17 Jun 2009 14:47:36 -0400
parents 8754d9d3bc2c
children f04f94067be0
files ChangeLog IcedTeaPlugin.cc netx/net/sourceforge/jnlp/NetxPanel.java plugin/icedtea/sun/applet/PluginAppletViewer.java
diffstat 4 files changed, 168 insertions(+), 32 deletions(-) [+]
line wrap: on
line diff
--- a/ChangeLog	Tue Jun 16 10:58:00 2009 -0400
+++ b/ChangeLog	Wed Jun 17 14:47:36 2009 -0400
@@ -1,3 +1,12 @@
+2009-06-17  Deepak Bhole  <dbhole@redhat.com>
+
+	* IcedTeaPlugin.cc: Fix race condition that led to segfault.
+	* plugin/icedtea/sun/applet/PluginAppletViewer.java: Fix a host of race
+	conditions brought about by the multi stage asynchronous initialization
+	design.
+	* netx/net/sourceforge/jnlp/NetxPanel.java: Account for case where handler
+	may be null due to an error above.
+
 2009-06-16  Omair Majid  <omajid@redhat.com>
 
 	* netx/javax/jnlp/SingleInstanceListener.java: New file.
--- a/IcedTeaPlugin.cc	Tue Jun 16 10:58:00 2009 -0400
+++ b/IcedTeaPlugin.cc	Wed Jun 17 14:47:36 2009 -0400
@@ -1065,6 +1065,7 @@
   gpointer window_handle;
   guint32 window_width;
   guint32 window_height;
+  PRBool is_active;
   // FIXME: nsCOMPtr.
   IcedTeaPluginFactory* factory;
   PRUint32 instance_identifier;
@@ -2447,6 +2448,7 @@
   nsCString destroyMessage (instanceIdentifierPrefix);
   destroyMessage += "destroy";
   factory->SendMessageToAppletViewer (destroyMessage);
+  is_active = PR_FALSE;
 
   return NS_OK;
 }
@@ -2475,7 +2477,9 @@
            long startTime = get_time_in_s();
            PRBool timedOut = PR_FALSE;
 
-           while (initialized == PR_FALSE && this->fatalErrorOccurred == PR_FALSE) 
+           while (initialized == PR_FALSE && 
+                  this->fatalErrorOccurred == PR_FALSE && 
+                  this->is_active == PR_FALSE) 
            {
                PROCESS_PENDING_EVENTS;
 
@@ -2638,7 +2642,9 @@
 
       long startTime = get_time_in_s();
       PRBool timedOut = PR_FALSE;
-      while (initialized == PR_FALSE && this->fatalErrorOccurred == PR_FALSE) 
+      while (initialized == PR_FALSE && 
+             this->fatalErrorOccurred == PR_FALSE && 
+             this->is_active == PR_FALSE) 
       {
           PROCESS_PENDING_EVENTS;
 
@@ -4115,6 +4121,7 @@
   liveconnect_window (0),
   initialized(PR_FALSE),
   fatalErrorOccurred(PR_FALSE),
+  is_active(PR_TRUE),
   instanceIdentifierPrefix ("")
 {
   PLUGIN_TRACE_INSTANCE ();
@@ -4134,6 +4141,7 @@
 
   nsresult result;
   PLUGIN_DEBUG_1ARG ("HERE 22: %d\n", liveconnect_window);
+
   // principalsArray, numPrincipals and securitySupports
   // are ignored by GetWindow.  See:
   //
@@ -4144,6 +4152,16 @@
   if (factory->proxyEnv != NULL)
     {
       PLUGIN_DEBUG_2ARG ("HERE 23: %d, %p\n", liveconnect_window, current_thread ());
+
+      // there is a bad race condition here where if the instance is active, 
+      // this code remains active after destruction.. so double check
+	  if (is_active != PR_TRUE)
+	  {
+		  PLUGIN_DEBUG_1ARG("Plugin %d is no longer active. Bypassing \
+                             GetWindow request.\n", instance_identifier);
+		  return;
+	  }
+
       result = factory->liveconnect->GetWindow(factory->proxyEnv,
                                                this,
                                                NULL, 0, NULL,
--- a/netx/net/sourceforge/jnlp/NetxPanel.java	Tue Jun 16 10:58:00 2009 -0400
+++ b/netx/net/sourceforge/jnlp/NetxPanel.java	Wed Jun 17 14:47:36 2009 -0400
@@ -144,7 +144,7 @@
     }
     
     public boolean isAlive() {
-        return handler.isAlive() && this.appletAlive;
+        return handler != null && handler.isAlive() && this.appletAlive;
     }
 }
 
--- a/plugin/icedtea/sun/applet/PluginAppletViewer.java	Tue Jun 16 10:58:00 2009 -0400
+++ b/plugin/icedtea/sun/applet/PluginAppletViewer.java	Wed Jun 17 14:47:36 2009 -0400
@@ -145,6 +145,8 @@
       */
      private static String defaultSaveFile = "Applet.ser";
      
+     private static enum PAV_INIT_STATUS {PRE_INIT, ACTIVE, INACTIVE};
+
      /**
       * The panel in which the applet is being displayed.
       */
@@ -168,17 +170,23 @@
  
      int identifier;
  
-     private static HashMap<Integer, PluginParseRequest> requests = new HashMap();
+     private static HashMap<Integer, PluginParseRequest> requests = 
+         new HashMap();
  
      // Instance identifier -> PluginAppletViewer object.
-     private static HashMap<Integer, PluginAppletViewer> applets = new HashMap();
+     private static HashMap<Integer, PluginAppletViewer> applets = 
+         new HashMap();
      
      private static PluginStreamHandler streamhandler;
      
      private static PluginCallRequestFactory requestFactory;
      
-     private static HashMap<Integer, String> siteCookies = new HashMap<Integer,String>();
-     
+     private static HashMap<Integer, String> siteCookies = 
+         new HashMap<Integer,String>();
+
+     private static HashMap<Integer, PAV_INIT_STATUS> status = 
+         new HashMap<Integer,PAV_INIT_STATUS>();
+
      private double proposedHeightFactor;
      private double proposedWidthFactor;
 
@@ -312,7 +320,7 @@
  	Applet a;
     while ((a = panel.getApplet()) == null && ((NetxPanel) panel).isAlive()) {
    	 try {
-   		 Thread.sleep(2000);
+   		 Thread.sleep(1000);
    		 PluginDebug.debug("Waiting for applet to initialize... ");
    	 } catch (InterruptedException ie) {
    		 ie.printStackTrace();
@@ -328,7 +336,8 @@
     PluginDebug.debug("Applet initialized");
 
     // Applet initialized. Find out it's classloader and add it to the list
-    String codeBase = doc.getProtocol() + "://" + doc.getHost();
+    String portComponent = doc.getPort() != -1 ? ":" + doc.getPort() : "";
+    String codeBase = doc.getProtocol() + "://" + doc.getHost() + portComponent;
 
     if (atts.get("codebase") != null) {
     	try {
@@ -373,6 +382,21 @@
         		 // may happen in independent threads
         		 
         		 synchronized(requests) {
+
+                     // Check if we should proceed with init 
+                     // (=> no if destroy was called after tag, but before 
+                     // handle)
+                     if (status.containsKey(identifier) &&
+                         status.get(identifier).equals(PAV_INIT_STATUS.INACTIVE)) {
+
+                         PluginDebug.debug("Inactive flag set. Refusing to initialize instance " + identifier);
+                         requests.remove(identifier);
+                         return;
+
+                     }
+
+        		     status.put(identifier, PAV_INIT_STATUS.PRE_INIT);
+        		     
         			 PluginParseRequest request = requests.get(identifier);
         			 if (request == null) {
         				 request = new PluginParseRequest();
@@ -393,6 +417,17 @@
         						 new StringReader(request.tag),
         						 new URL(request.documentbase));
         				 requests.remove(identifier);
+        				 
+        				 // Panel initialization cannot be aborted mid-way. 
+        				 // Once it is initialized, double check to see if this 
+        				 // panel needs to stay around..
+        				 if (status.get(identifier).equals(PAV_INIT_STATUS.INACTIVE)) {
+        				     PluginDebug.debug("Inactive flag set. Destroying applet instance " + identifier);
+        				     applets.get(identifier).handleMessage(-1, "destroy");
+        				 } else {
+        				     status.put(identifier, PAV_INIT_STATUS.ACTIVE);
+        				 }
+
         			 } else {
         				 PluginDebug.debug ("REQUEST HANDLE NOT SET: " + request.handle + ". BYPASSING");
         			 }
@@ -400,6 +435,21 @@
         		 
              } else if (message.startsWith("handle")) {
             	 synchronized(requests) {
+            	     
+            	     // Check if we should proceed with init 
+            	     // (=> no if destroy was called after handle, but before 
+            	     // tag)
+            	     if (status.containsKey(identifier) &&
+                         status.get(identifier).equals(PAV_INIT_STATUS.INACTIVE)) {
+
+            	         PluginDebug.debug("Inactive flag set. Refusing to initialize instance " + identifier);
+            	         requests.remove(identifier);
+            	         return;
+
+            	     }
+
+            	     status.put(identifier, PAV_INIT_STATUS.PRE_INIT);
+
             		 PluginParseRequest request = requests.get(identifier);
             		 if (request == null) {
             			 request = new PluginParseRequest();
@@ -418,6 +468,17 @@
             			 requests.remove(identifier);
             			 PluginDebug.debug ("REQUEST HANDLE, DONE PARSING " +
             					 Thread.currentThread());
+
+                         // Panel initialization cannot be aborted mid-way. 
+                         // Once it is initialized, double check to see if this 
+                         // panel needs to stay around..
+                         if (status.get(identifier).equals(PAV_INIT_STATUS.INACTIVE)) {
+                             PluginDebug.debug("Inactive flag set. Destroying applet instance " + identifier);
+                             applets.get(identifier).handleMessage(-1, "destroy");
+                         } else {
+                             status.put(identifier, PAV_INIT_STATUS.ACTIVE);
+                         }
+
             		 } else {
             			 PluginDebug.debug ("REQUEST TAG NOT SET: " + request.tag + ". BYPASSING");
             		 }
@@ -433,12 +494,57 @@
                  // Always set the cookie -- even if it is null
                  siteCookies.put(identifier, cookieStr);
              } else {
-                 PluginDebug.debug ("HANDLING MESSAGE " + message + " instance " + identifier + " " + Thread.currentThread());
+                 PluginDebug.debug ("Handling message: " + message + " instance " + identifier + " " + Thread.currentThread());
+
+                 // Destroy may be called while initialization is still going 
+                 // on. We therefore special case it.
+                 if (!applets.containsKey(identifier) && message.equals("destroy")) {
+
+                     // Set the status to inactive right away. Doesn't matter if it 
+                     // gets clobbered during init. due to a race. That is what the 
+                     // double check below is for.  
+                     PluginDebug.debug("Destroy called during initialization. Delaying destruction.");
+                     status.put(identifier, PAV_INIT_STATUS.INACTIVE);
+
+                     // We have set the flags. We now lock what stage 1 and 2 
+                     // lock on, and force a synchronous status check+action. 
+                     synchronized (requests) {
+                         // re-check (inside lock) if the applet is 
+                         // initialized at this point. 
+                         if (applets.containsKey(identifier)) {
+                             PluginDebug.debug("Init done. destroying normally.");
+                             applets.get(identifier).handleMessage(reference, message);
+                         } else {
+                         }
+                     } // unlock
+
+                     // we're done here
+                     return;
+                 }
+
+                 // For messages other than destroy, 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);
              }
          } catch (Exception e) {
-             throw new RuntimeException("Failed to handle message: " + message + " " +
-                                         Thread.currentThread(), e);
+
+             // 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);
+
+             throw new RuntimeException("Failed to handle message: " + 
+                     message + " for instance " + identifier + " " +  
+                     Thread.currentThread(), e);
          }
      }
  
@@ -487,6 +593,7 @@
 			}
          } 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
              // object should belong to?
@@ -1291,26 +1398,28 @@
       * the last applet.
       */
      void appletClose() {
- 
- 	// The caller thread is event dispatch thread, so
- 	// spawn a new thread to avoid blocking the event queue
- 	// when calling appletShutdown.
- 	//
- 	final AppletPanel p = panel;
- 
- 	new Thread(new Runnable()
- 	{
- 	    public void run()
- 	    {
-     		appletShutdown(p);
- 		appletPanels.removeElement(p);
- 		dispose();
- 
- 		if (countApplets() == 0) {
- 		    appletSystemExit();
- 		}
- 	    }
- 	}).start();
+
+         // The caller thread is event dispatch thread, so
+         // spawn a new thread to avoid blocking the event queue
+         // when calling appletShutdown.
+         //
+         final AppletPanel p = panel;
+
+         new Thread(new Runnable()
+         {
+             public void run()
+             {
+                 appletShutdown(p);
+                 appletPanels.removeElement(p);
+                 dispose();
+
+                 if (countApplets() == 0) {
+                     appletSystemExit();
+                 }
+             }
+         }).start();
+
+         status.put(identifier, PAV_INIT_STATUS.INACTIVE);
      }
  
      /**