Adding a startup regression test.

tests/startup_regression/startup_regression.py

+#!/usr/bin/python
+# Copyright (c) 2012 The Native Client Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import codecs
+import hashlib
+import json
+import optparse
+import os
+import re
+import subprocess
+import sys
+import tempfile
+import threading
+import time
+import zipfile
+
+
+KNOWN_BAD = set([

[Nick Bray, 2012/04/02 20:31:41]

This is going to be visible in a publicly viewable repo.  Weren't you concerned
about spewing app IDs?

[bradn, 2012/04/02 22:43:59]

So actually mainly I'm worried about app ids.
These are sha1's. If anything I'm thinking of going back to the other and
calling out only sha1s for known bad ones (there were 2 in the other).

+  # Bad manifest
+  '2f97cec9f13b0f774d1f49490f26f32213e4e0a5',
+  'ced1fea90b71b0a8da08c1a1e6cb35975cc84f52',
+  '3d6832749c8c1346c65b30f4b191930dec5f04a3',
+  '0937b653af5553856532454ec340d0e0075bc0b4',
+  '09ffe3793113fe564b71800a5844189c00bd8210',
+  '81a4a3de69dd4ad169b1d4a7268b44c78ea5ffa8',
+  '612a5aaa821b4b636168025f027e721c0f046e7c',
+  '14f389a8c406d60e0fc05a1ec0189a652a1f006e',
+  'a8aa42d699dbef3e1403e4fdc49325e89a91f653',
+  'c6d40d4f3c8dccc710d8c09bfd074b2d20a504d2',
+  # Bad permissions
+  '8de65668cc7280ffb70ffd2fa5b2a22112156966',
+  # Snap
+  'b458cd57c8b4e6c313b18f370fad59779f573afc',
+  # No nacl module
+  '57be161e5ff7011d2283e507a70f9005c448002b',
+  '4beecff67651f13e013c12a5bf3661041ded323c',
+  '1f861c0d8c173b64df3e70cfa1a5cd710ba59430',
+  'cfd62adf6790eed0520da2deb2246fc02e70c57e',
+  ])
+
+
+SCRIPT_DIR = os.path.dirname(os.path.abspath(__file__))
+TESTS_DIR = os.path.dirname(SCRIPT_DIR)
+NACL_DIR = os.path.dirname(TESTS_DIR)
+
+
+# Imports from the build directory.
+sys.path.insert(0, os.path.join(NACL_DIR, 'build'))
+import download_utils

[Nick Bray, 2012/04/02 20:31:41]

Move import hackery above KNOWN_BAD - keep it grouped.

[bradn, 2012/04/02 22:43:59]

Done.

+
+
+def GsutilCopySilent(src, dst):
+  """Invoke gsutil cp, swallowing the output, with retry.
+
+  Args:
+    src: src url.
+    dst: dst path.
+  """
+  for _ in range(3):
+    env = os.environ.copy()
+    env['PATH'] = '/b/build/scripts/slave' + os.pathsep + env['PATH']

[Nick Bray, 2012/04/02 20:31:41]

* Lift env creation out of loop - makes intent more obvious.
* Document why you typically need to retry?
* Optional: Add sleep?

[bradn, 2012/04/02 22:43:59]

Done.

+    process = subprocess.Popen(
+        ['gsutil', 'cp', src, dst],
+        env=env, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+    process_stdout, process_stderr = process.communicate()
+    if process.returncode == 0:
+      return
+  print 'Unexpected return code: %s' % process.returncode
+  print '>>> STDOUT'
+  print process_stdout
+  print '>>> STDERR'
+  print process_stderr
+  print '-' * 70
+  sys.exit(1)

[Nick Bray, 2012/04/02 20:31:41]

Very optional: I am not a huge fan of exiting inside a function.  There's
nothing wrong, pre se, but my opinion is that throwing user-defined 'failiure'
exceptions (which is almost equivalent) is more future proof - how you handle
failure at an outer scope may change, etc.  (See buildbox_standard.py) Not a
very convincing argument, it's mostly instinct.

[bradn, 2012/04/02 22:43:59]

Done, due to later unrelated refactor.

+
+
+def DownloadTotalList(list_filename):

[Nick Bray, 2012/04/02 20:31:41]

Bad name.  List of what?  Total of what?

[bradn, 2012/04/02 22:43:59]

Done.

+  """Download list of all archived files.
+
+  Args:
+    list_filename: destination filename (kept around for debugging).
+  """
+  GsutilCopySilent('gs://nativeclient-snaps/naclapps.all', list_filename)
+  fh = open(list_filename)
+  filenames = fh.read().splitlines()
+  fh.close()
+  return [f for f in filenames if f.endswith('.crx')]
+
+
+def DownloadFile(src_path, dst_filename):

[Nick Bray, 2012/04/02 20:31:41]

Bad name.  DownloadFileFromSnapshot?

[bradn, 2012/04/02 22:43:59]

Done.

+  """Download a file from our snapshot.
+
+  Args:
+    src_path: datastore relative path to download from.
+    dst_filename: destination filename.
+  """
+  GsutilCopySilent('gs://nativeclient-snaps/%s' % src_path, dst_filename)
+
+
+def Sha1Sum(path):

[Nick Bray, 2012/04/02 20:31:41]

Sha1Digest?  Sha1FileDigest?

[bradn, 2012/04/02 22:43:59]

Done.

+  """Determine the sha1 hash of a file's contents given its path."""
+  m = hashlib.sha1()
+  fh = open(path, 'rb')
+  m.update(fh.read())
+  fh.close()
+  return m.hexdigest()
+
+
+def Hex2Alpha(ch):
+  """Convert a hexadecimal digit from 0-9 / a-f to a-p.
+
+  Args:
+    ch: a character in 0-9 / a-f.
+  Returns:
+    A character in a-p.
+  """
+  if ch >= '0' and ch <= '9':
+    return chr(ord(ch) - ord('0') + ord('a'))
+  else:
+    return chr(ord(ch) + 10)
+
+
+def ChromeAppIdFromPath(path):
+  """Converts a path to the corrisponding chrome app id.

[Nick Bray, 2012/04/02 20:31:41]

Path, in what context?  On disk?  In the web store?  In the extension directory?

It would be nice to know where you pulled this algorithm from and why you need
it.

[bradn, 2012/04/02 22:43:59]

Done.

+
+  Args:
+    path: Path to an unpacked extension.
+  Returns:
+    A 32 character chrome extension app id.
+  """
+  hasher = hashlib.sha256()
+  hasher.update(os.path.realpath(path))
+  hexhash = hasher.hexdigest()[:32]
+  return ''.join([Hex2Alpha(ch) for ch in hexhash])
+
+
+def TestAppStartup(options, crx_path, app_path, profile_path):

[Nick Bray, 2012/04/02 20:31:41]

Big function.  Is there a clean way to modularize it?

[bradn, 2012/04/02 22:43:59]

Done.

+  """Run the validator on a nexe, check if the result is expected.
+
+  Args:
+    options: bag of options.
+    crx_path: path to the crx.
+    app_path: path to the extracted crx.
+    profile_path: path to a temporary profile dir.
+  """
+  manifest = LoadManifest(app_path)
+  start_path = manifest.get('app', {}).get('launch', {}).get('local_path')
+  if not start_path:
+    print '-' * 70
+    print 'Testing: %s' % crx_path
+    print 'Browser: %s' % options.browser
+    print 'BAD MANIFEST!'
+    print '-' * 70
+    # Halt on first failure.
+    sys.exit(1)
+  start_url = 'chrome-extension://%s/%s' % (
+      ChromeAppIdFromPath(app_path), start_path)
+  cmd = [options.browser,
+      '--enable-nacl',
+      '--load-extension=' + app_path,
+      '--user-data-dir=' + profile_path, start_url]
+  process = subprocess.Popen(cmd,

[Nick Bray, 2012/04/02 20:31:41]

For example, this is essentially subprocess.communicate with a time out.  Sounds
like a function to me.

[bradn, 2012/04/02 22:43:59]

Done.

+      stdout=subprocess.PIPE,
+      stderr=subprocess.PIPE)
+  def GatherOutput(fh, dst):
+    dst.append(fh.read())
+  # Gather stdout.
+  stdout_output = []
+  stdout_thread = threading.Thread(
+      target=GatherOutput, args=(process.stdout, stdout_output))
+  stdout_thread.setDaemon(True)
+  stdout_thread.start()
+  # Gather stderr.
+  stderr_output = []
+  stderr_thread = threading.Thread(
+      target=GatherOutput, args=(process.stderr, stderr_output))
+  stderr_thread.setDaemon(True)
+  stderr_thread.start()
+  # Wait for a small span for the app to load.
+  time.sleep(options.duration)
+  process.kill()
+  time.sleep(1)

[Nick Bray, 2012/04/02 20:31:41]

Why sleep?

[bradn, 2012/04/02 22:43:59]

Oops, switched to wait.

+  process.poll()
+  # Join up.
+  stdout_thread.join()
+  stderr_thread.join()
+  # Pick out result.
+  process_stdout = stdout_output[0]

[Nick Bray, 2012/04/02 20:31:41]

Scraping the output will not work on Windows.  We probably will want to run on
Windows, too.  A problem for tomorrow, but you might want to check if someone
tried to run this test on Windows and print a helpful error that explains why
that won't work.  

[bradn, 2012/04/02 22:43:59]

Done.

+  process_stderr = stderr_output[0]

[Nick Bray, 2012/04/02 20:31:41]

Scraping for failiures => function (but not printing the failiure)

[bradn, 2012/04/02 22:43:59]

Added --verbose flag that does emit it.

+  # Check for errors we don't like.
+  failure = None
+  if 'NaClMakePcrelThunk:' not in process_stderr:
+    failure = 'nacl module not started'
+  if 'NaCl process exited with' in process_stderr:
+    failure = 'nacl module crashed'
+  errs = re.findall(':ERROR:[^\n]+', process_stderr)
+  for err in errs:
+    if ('extension_prefs.cc' not in err and
+        'gles2_cmd_decoder.cc' not in err):
+      failure = 'unknown error: ' + err
+      break
+  # Check if result is what we expect.
+  if failure:
+    print '-' * 70
+    print 'Testing: %s' % crx_path
+    print 'Browser: %s' % options.browser
+    print 'Failure: %s' % failure
+    print '>>> STDOUT'
+    print process_stdout
+    print '>>> STDERR'
+    print process_stderr
+    print '-' * 70
+    # Halt on first failure.
+    sys.exit(1)
+
+
+def LoadManifest(app_path):
+  try:
+    manifest_data = codecs.open(os.path.join(app_path, 'manifest.json'),
+                               'r', encoding='utf-8').read()
+    manifest_data = manifest_data.replace('\r', '')
+    manifest_data = manifest_data.replace(u'\ufeff', '')

[Nick Bray, 2012/04/02 20:31:41]

What are these characters?  Document.

[bradn, 2012/04/02 22:43:59]

Done.

+    manifest_data = manifest_data.replace(u'\uffee', '')
+    return json.loads(manifest_data)
+  except:
+    return {}

[Nick Bray, 2012/04/02 20:31:41]

This seems a little sketchy.  Why return an empty dict?

[bradn, 2012/04/02 22:43:59]

Simplifies the logic of checking for a nested key within it.
ie.
x.get('foo', {}).get('bar', {}).get('baz')

+
+
+def IsBad(path):
+  """Checks a blacklist to decide if we should startup test this app.
+
+  Args:
+    path: path to the nexe.
+  Returns:
+    Boolean indicating if we should test this app.
+  """
+  return os.path.splitext(os.path.basename(path))[0] in KNOWN_BAD
+
+
+def CachedPath(options, filename):
+  """Find the full path of a cached file, a cache root relative path.
+
+  Args:
+    options: bags of options.
+    filename: filename relative to the top of the download url / cache.
+  Returns:
+    Absolute path of where the file goes in the cache.
+  """
+  return os.path.join(options.cache_dir, 'nacl_startup_test_cache', filename)
+
+
+def Sha1FromFilename(filename):

[Nick Bray, 2012/04/02 20:31:41]

Are there functions here that you should be sharing with the validator script?

[bradn, 2012/04/02 22:43:59]

Done.

+  """Get the expected sha1 of a file path.
+
+  Throughout we use the convention that files are store to a name of the form:
+    <path_to_file>/<sha1hex>[.<some_extention>]
+  This function extracts the expected sha1.
+
+  Args:
+    filename: filename to extract.
+  Returns:
+    Excepted sha1.
+  """
+  return os.path.splitext(os.path.basename(filename))[0]
+
+
+def PrimeCache(options, filename):

[Nick Bray, 2012/04/02 20:31:41]

Dito.

[bradn, 2012/04/02 22:43:59]

Done.

+  """Attempt to add a file to the cache directory if its not already there.
+
+  Args:
+    options: bag of options.
+    filename: filename relative to the top of the download url / cache.
+  """
+  dpath = CachedPath(options, filename)
+  if not os.path.exists(dpath) or Sha1Sum(dpath) != Sha1FromFilename(filename):
+    # Try to make the directory, fail is ok, let the download fail instead.
+    try:
+      os.makedirs(os.path.basename(dpath))
+    except OSError:
+      pass
+    DownloadFile(filename, dpath)
+
+
+def ExtractFromCache(options, source, dest):
+  """Extract a crx from the cache.
+
+  Args:
+    options: bag of options.
+    source: crx file to extract (cache relative).
+    dest: location to extract to.
+  """
+  assert not os.path.exists(dest)

[Nick Bray, 2012/04/02 20:31:41]

If you have an assert, have a message to help explain it.  dest, in this case,
would be sufficient.

[bradn, 2012/04/02 22:43:59]

Done.

+  dpath = CachedPath(options, source)
+  assert os.path.exists(dpath)
+  zf = zipfile.ZipFile(dpath, 'r')
+  os.makedirs(dest)
+  for info in zf.infolist():
+    tpath = os.path.join(dest, info.filename)

[Nick Bray, 2012/04/02 20:31:41]

Pwnage: Zipfile is from an untrusted source, filename could be an absolute path,
join discards everything before an absolute path, attacker could cause you to
overwrite an arbitrary file.

[bradn, 2012/04/02 22:43:59]

Done.

+    if info.filename.endswith('/'):
+      os.makedirs(tpath)
+    else:
+      zf.extract(info, dest)
+  zf.close()
+
+
+def TestApps(options, work_dir):
+  """Test a browser on a corpus of crxs.
+
+  Args:
+    options: bag of options.
+    work_dir: directory to operate in.
+  """
+  profile_path = os.path.join(work_dir, 'profile_temp')
+  app_path = os.path.join(work_dir, 'app_temp')
+
+  list_filename = os.path.join(work_dir, 'naclapps.all')
+  filenames = DownloadTotalList(list_filename)
+  filenames = filenames[24:]
+
+  count = 0
+  start = time.time()
+  count = len(filenames)
+  for index, filename in enumerate(filenames):
+    tm = time.time()
+    if index > 0:

[Nick Bray, 2012/04/02 20:31:41]

ETA calculation and / or header printing in function?

[bradn, 2012/04/02 22:43:59]

Done.

+      eta = (count - index) * (tm - start) / index
+      eta_minutes = int(eta / 60)
+      eta_seconds = int(eta - eta_minutes * 60)
+      eta_str = ' (ETA %d:%02d)' % (eta_minutes, eta_seconds)
+    else:
+      eta_str = ''
+    print 'Processing %d of %d%s...' % (index + 1, count, eta_str)
+    # Skip if known bad.
+    if IsBad(filename):
+      continue
+    PrimeCache(options, filename)
+    # Stop here if downloading only.
+    if options.download_only:
+      continue
+    # Unzip the app.
+    ExtractFromCache(options, filename, app_path)
+    try:
+      TestAppStartup(options, filename, app_path, profile_path)
+      count += 1
+    finally:
+      download_utils.RemoveDir(app_path)
+      download_utils.RemoveDir(profile_path)
+  print 'Ran tests on %d of %d CRXs' % (count, len(filenames))
+  print 'SUCCESS'
+
+
+def Main():
+  # Decide a default cache directory.
+  # Prefer /b (for the bots)
+  # Failing that, use scons-out.
+  # Failing that, use the current users's home dir.
+  default_cache_dir = '/b'

[Nick Bray, 2012/04/02 20:31:41]

Another bit of code that might do well shared.

[bradn, 2012/04/02 22:43:59]

Done.

+  if not os.path.isdir(default_cache_dir):
+    default_cache_dir = os.path.join(NACL_DIR, 'scons-out')
+  if not os.path.isdir(default_cache_dir):
+    default_cache_dir = os.path.expanduser('~/')
+  default_cache_dir = os.path.abspath(default_cache_dir)
+  assert os.path.isdir(default_cache_dir)
+
+  parser = optparse.OptionParser()
+  parser.add_option(
+      '--cache-dir', dest='cache_dir', default=default_cache_dir,
+      help='directory to cache downloads in')
+  parser.add_option(
+      '--download-only', dest='download_only',
+      default=False, action='store_true',
+      help='download to cache without running the tests')
+  parser.add_option(
+      '--duration', dest='duration', default=30,
+      help='how long to run each app for')
+  parser.add_option(
+      '--browser', dest='browser',
+      help='browser to run')
+  options, args = parser.parse_args()
+  if args:
+    parser.error('unused arguments')
+  if not options.download_only:
+    if not options.browser:
+      parser.error('no browser specified')
+
+  work_dir = tempfile.mkdtemp(suffix='startup_crxs', prefix='tmp')
+  work_dir = os.path.realpath(work_dir)
+  try:
+    TestApps(options, work_dir)
+  finally:
+    download_utils.RemoveDir(work_dir)
+
+
+if __name__ == '__main__':
+  Main()