2010-04-28 Eric Seidel <eric@webkit.org>
authoreric@webkit.org <eric@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 28 Apr 2010 21:51:34 +0000 (21:51 +0000)
committereric@webkit.org <eric@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 28 Apr 2010 21:51:34 +0000 (21:51 +0000)
        Reviewed by David Levin.

        Audit all uses of subprocess in webkitpy
        https://bugs.webkit.org/show_bug.cgi?id=38284

        After further discussions with Jeffrey Yasskin
        about http://bugs.python.org/issue2320
        and related issues of using subprocess from
        multiple threads, I have learned that subprocess
        is known to be non-threadsafe through recent
        Python 2.7 builds.

        I'm attempting to lessen our exposure to these
        subprocess bugs by auditing each use of subprocess
        in webkitpy.  I did not find any unsafe calls
        in my audit, but I did remove numerous unneeded
        import subprocess lines.

        * Scripts/webkitpy/common/checkout/api.py:
        * Scripts/webkitpy/common/net/bugzilla.py:
        * Scripts/webkitpy/common/system/deprecated_logging_unittest.py:
        * Scripts/webkitpy/common/system/user.py:
        * Scripts/webkitpy/layout_tests/port/base.py:
        * Scripts/webkitpy/layout_tests/port/chromium_linux.py:
        * Scripts/webkitpy/layout_tests/port/chromium_mac.py:
        * Scripts/webkitpy/layout_tests/port/chromium_mac_unittest.py: Added.
        * Scripts/webkitpy/layout_tests/port/chromium_win.py:
        * Scripts/webkitpy/layout_tests/port/gtk.py:
        * Scripts/webkitpy/layout_tests/port/mac.py:
        * Scripts/webkitpy/layout_tests/port/qt.py:
        * Scripts/webkitpy/layout_tests/port/webkit.py:
        * Scripts/webkitpy/layout_tests/port/win.py:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@58434 268f45cc-cd09-0410-ab3c-d52691b4dbfc

15 files changed:
WebKitTools/ChangeLog
WebKitTools/Scripts/webkitpy/common/checkout/api.py
WebKitTools/Scripts/webkitpy/common/net/bugzilla.py
WebKitTools/Scripts/webkitpy/common/system/deprecated_logging_unittest.py
WebKitTools/Scripts/webkitpy/common/system/user.py
WebKitTools/Scripts/webkitpy/layout_tests/port/base.py
WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_linux.py
WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_mac.py
WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_mac_unittest.py [new file with mode: 0644]
WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win.py
WebKitTools/Scripts/webkitpy/layout_tests/port/gtk.py
WebKitTools/Scripts/webkitpy/layout_tests/port/mac.py
WebKitTools/Scripts/webkitpy/layout_tests/port/qt.py
WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py
WebKitTools/Scripts/webkitpy/layout_tests/port/win.py

index 200bf95..1d4fada 100644 (file)
@@ -1,3 +1,38 @@
+2010-04-28  Eric Seidel  <eric@webkit.org>
+
+        Reviewed by David Levin.
+
+        Audit all uses of subprocess in webkitpy
+        https://bugs.webkit.org/show_bug.cgi?id=38284
+
+        After further discussions with Jeffrey Yasskin
+        about http://bugs.python.org/issue2320
+        and related issues of using subprocess from
+        multiple threads, I have learned that subprocess
+        is known to be non-threadsafe through recent
+        Python 2.7 builds.
+
+        I'm attempting to lessen our exposure to these
+        subprocess bugs by auditing each use of subprocess
+        in webkitpy.  I did not find any unsafe calls
+        in my audit, but I did remove numerous unneeded
+        import subprocess lines.
+
+        * Scripts/webkitpy/common/checkout/api.py:
+        * Scripts/webkitpy/common/net/bugzilla.py:
+        * Scripts/webkitpy/common/system/deprecated_logging_unittest.py:
+        * Scripts/webkitpy/common/system/user.py:
+        * Scripts/webkitpy/layout_tests/port/base.py:
+        * Scripts/webkitpy/layout_tests/port/chromium_linux.py:
+        * Scripts/webkitpy/layout_tests/port/chromium_mac.py:
+        * Scripts/webkitpy/layout_tests/port/chromium_mac_unittest.py: Added.
+        * Scripts/webkitpy/layout_tests/port/chromium_win.py:
+        * Scripts/webkitpy/layout_tests/port/gtk.py:
+        * Scripts/webkitpy/layout_tests/port/mac.py:
+        * Scripts/webkitpy/layout_tests/port/qt.py:
+        * Scripts/webkitpy/layout_tests/port/webkit.py:
+        * Scripts/webkitpy/layout_tests/port/win.py:
+
 2010-04-28  Darin Adler  <darin@apple.com>
 
         Ignore a directory the Python tools creates.
index 595c3d5..a5ac939 100644 (file)
@@ -27,7 +27,6 @@
 # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 import os
-import subprocess
 import StringIO
 
 from webkitpy.common.checkout.changelog import ChangeLog
index d994662..4311a00 100644 (file)
@@ -33,7 +33,6 @@
 import os.path
 import re
 import StringIO
-import subprocess
 
 from datetime import datetime # used in timestamp()
 
index 2b71803..3778162 100644 (file)
@@ -27,7 +27,6 @@
 # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 import os
-import subprocess
 import StringIO
 import tempfile
 import unittest
index 12cc524..64995bb 100644 (file)
@@ -62,6 +62,7 @@ class User(object):
     def edit(self, files):
         editor = os.environ.get("EDITOR") or "vi"
         args = shlex.split(editor)
+        # Note: Not thread safe: http://bugs.python.org/issue2320
         subprocess.call(args + files)
 
     def page(self, message):
index cac6722..bf46c9f 100644 (file)
@@ -35,7 +35,6 @@ import difflib
 import errno
 import os
 import shlex
-import subprocess
 import sys
 import time
 
@@ -148,17 +147,13 @@ class Port(object):
 
         result = True
         try:
-            if subprocess.call(cmd) == 0:
+            if self._executive.run_command(cmd, return_exit_code=True) == 0:
                 return False
         except OSError, e:
             if e.errno == errno.ENOENT or e.errno == errno.EACCES:
                 _compare_available = False
             else:
                 raise e
-        except ValueError:
-            # work around a race condition in Python 2.4's implementation
-            # of subprocess.Popen. See http://bugs.python.org/issue1199282 .
-            pass
         return result
 
     def diff_text(self, expected_text, actual_text,
index b821155..a01bd14 100644 (file)
@@ -31,9 +31,7 @@
 
 import logging
 import os
-import platform
 import signal
-import subprocess
 
 import chromium
 
@@ -122,6 +120,7 @@ class ChromiumLinuxPort(chromium.ChromiumPort):
             _log.error('    Please install using: "sudo apt-get install '
                        'wdiff"')
             _log.error('')
+        # FIXME: The ChromiumMac port always returns True.
         return result
 
     def _path_to_apache(self):
index 44fd38b..0d1d27b 100644 (file)
@@ -33,10 +33,11 @@ import logging
 import os
 import platform
 import signal
-import subprocess
 
 import chromium
 
+from webkitpy.common.system.executive import Executive
+
 _log = logging.getLogger("webkitpy.layout_tests.port.chromium_mac")
 
 
@@ -99,16 +100,12 @@ class ChromiumMacPort(chromium.ChromiumPort):
         return self.path_from_chromium_base('xcodebuild', *comps)
 
     def _check_wdiff_install(self):
-        # FIXME: This should use Executive.
-        f = open(os.devnull, 'w')
-        rcode = 0
         try:
-            rcode = subprocess.call(['wdiff'], stderr=f)
+            # We're ignoring the return and always returning True
+            self._executive.run_command([self._path_to_wdiff()], error_handler=Executive.ignore_error)
         except OSError:
             _log.warning('wdiff not found. Install using MacPorts or some '
                          'other means')
-            pass
-        f.close()
         return True
 
     def _lighttpd_path(self, *comps):
diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_mac_unittest.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_mac_unittest.py
new file mode 100644 (file)
index 0000000..d63faa0
--- /dev/null
@@ -0,0 +1,40 @@
+# Copyright (C) 2010 Google Inc. All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+#    * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#    * Redistributions in binary form must reproduce the above
+# copyright notice, this list of conditions and the following disclaimer
+# in the documentation and/or other materials provided with the
+# distribution.
+#    * Neither the name of Google Inc. nor the names of its
+# contributors may be used to endorse or promote products derived from
+# this software without specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+import chromium_mac
+import unittest
+
+from webkitpy.thirdparty.mock import Mock
+
+
+class ChromiumMacPortTest(unittest.TestCase):
+
+    def test_check_wdiff_install(self):
+        port = chromium_mac.ChromiumMacPort()
+        # Currently is always true, just logs if missing.
+        self.assertTrue(port._check_wdiff_install())
index 8cc8b7d..59dc1d9 100644 (file)
@@ -30,7 +30,6 @@
 
 import logging
 import os
-import subprocess
 
 from webkitpy.layout_tests.port.webkit import WebKitPort
 
index f321fc5..350b088 100644 (file)
 
 import logging
 import os
-import pdb
 import platform
-import re
-import shutil
 import signal
-import subprocess
-import sys
-import time
-import webbrowser
 
 import webkitpy.common.system.ospath as ospath
 import webkitpy.layout_tests.port.server_process as server_process
index 58e90dc..9032a24 100644 (file)
@@ -30,7 +30,6 @@
 
 import logging
 import os
-import subprocess
 import signal
 
 from webkitpy.layout_tests.port.webkit import WebKitPort
index c48e450..ada83ce 100644 (file)
@@ -35,12 +35,9 @@ from __future__ import with_statement
 import codecs
 import logging
 import os
-import pdb
-import platform
 import re
 import shutil
 import signal
-import subprocess
 import sys
 import time
 import webbrowser
@@ -388,7 +385,6 @@ class WebKitDriver(base.Driver):
             command += "'" + image_hash
         command += "\n"
 
-        # pdb.set_trace()
         self._server_process.write(command)
 
         have_seen_content_type = False
index f7b3b78..3b7a817 100644 (file)
@@ -30,7 +30,6 @@
 
 import logging
 import os
-import subprocess
 
 from webkitpy.layout_tests.port.webkit import WebKitPort