SI-7775 Exclude nulls when iterating sys props

The previous fix to deal with concurrent modification of system
properties doesn't handle null results introduced when a property
is removed.

This commit filters nulls for safety, and also adds a `names`
method to `sys.SystemProperties`.

The test is upgraded.

5 files changed, 58 insertions, 6 deletions

diff --git a/src/compiler/scala/tools/cmd/Property.scala b/src/compiler/scala/tools/cmd/Property.scala
index b1d951a5c4..e6262a7e40 100644
--- a/src/compiler/scala/tools/cmd/Property.scala
+++ b/src/compiler/scala/tools/cmd/Property.scala
@@ -9,6 +9,7 @@ package cmd
 import nsc.io._
 import java.util.Properties
 import java.io.FileInputStream
+import scala.sys.SystemProperties
 
 /** Contains logic for translating a property key/value pair into
  *  equivalent command line arguments.  The default settings will
@@ -58,7 +59,7 @@ trait Property extends Reference {
     returning(new Properties)(_ load new FileInputStream(file.path))
 
   def systemPropertiesToOptions: List[String] =
-    propertiesToOptions(System.getProperties)
+    propertiesToOptions(new SystemProperties().toList)
 
   def propertiesToOptions(file: File): List[String] =
     propertiesToOptions(loadProperties(file))
diff --git a/src/compiler/scala/tools/reflect/WrappedProperties.scala b/src/compiler/scala/tools/reflect/WrappedProperties.scala
index 523287fc66..348d000d15 100644
--- a/src/compiler/scala/tools/reflect/WrappedProperties.scala
+++ b/src/compiler/scala/tools/reflect/WrappedProperties.scala
@@ -30,9 +30,10 @@ trait WrappedProperties extends PropertiesTrait {
   def systemProperties: List[(String, String)] = {
     import scala.collection.JavaConverters._
     wrap {
+      // SI-7269,7775 Avoid `ConcurrentModificationException` and nulls if another thread modifies properties
       val props = System.getProperties
-      // SI-7269 Be careful to avoid `ConcurrentModificationException` if another thread modifies the properties map
-      props.stringPropertyNames().asScala.toList.map(k => (k, props.get(k).asInstanceOf[String]))
+      val it = props.stringPropertyNames().asScala.iterator map (k => (k, props getProperty k)) filter (_._2 ne null)
+      it.toList
     } getOrElse Nil
   }
 }
diff --git a/src/compiler/scala/tools/util/PathResolver.scala b/src/compiler/scala/tools/util/PathResolver.scala
index 8e5b1e0a5c..f122437b63 100644
--- a/src/compiler/scala/tools/util/PathResolver.scala
+++ b/src/compiler/scala/tools/util/PathResolver.scala
@@ -52,7 +52,7 @@ object PathResolver {
    */
   object Environment {
     private def searchForBootClasspath =
-      systemProperties find (_._1 endsWith ".boot.class.path") map (_._2) getOrElse ""
+      systemProperties collectFirst { case (k, v) if k endsWith ".boot.class.path" => v } getOrElse ""
 
     /** Environment variables which java pays attention to so it
      *  seems we do as well.
diff --git a/src/library/scala/sys/SystemProperties.scala b/src/library/scala/sys/SystemProperties.scala
index d2ebf8c044..6f8b13a89b 100644
--- a/src/library/scala/sys/SystemProperties.scala
+++ b/src/library/scala/sys/SystemProperties.scala
@@ -35,8 +35,15 @@ extends mutable.AbstractMap[String, String]
   override def empty = new SystemProperties
   override def default(key: String): String = null
 
-  def iterator: Iterator[(String, String)] =
-    wrapAccess(System.getProperties().asScala.iterator) getOrElse Iterator.empty
+  def iterator: Iterator[(String, String)] = wrapAccess {
+    val ps = System.getProperties()
+    names map (k => (k, ps getProperty k)) filter (_._2 ne null)
+  } getOrElse Iterator.empty
+
+  def names: Iterator[String] = wrapAccess (
+    System.getProperties().stringPropertyNames().asScala.iterator
+  ) getOrElse Iterator.empty
+
   def get(key: String) =
     wrapAccess(Option(System.getProperty(key))) flatMap (x => x)
   override def contains(key: String) =
diff --git a/test/files/run/t7775.scala b/test/files/run/t7775.scala
index 48b0d89974..bc69064e17 100644
--- a/test/files/run/t7775.scala
+++ b/test/files/run/t7775.scala
@@ -1,3 +1,45 @@
+import scala.concurrent._, duration._
+import ExecutionContext.Implicits.global
+import scala.tools.reflect.WrappedProperties.AccessControl._
+import java.util.concurrent.CyclicBarrier
+
+object Test extends App {
+  @volatile var done = false
+  val barrier = new CyclicBarrier(2)
+
+  val probe = Future {
+    val attempts     = 1024          // previously, failed after a few
+    def fail(i: Int) = s"Failed at $i"
+    barrier.await()
+    for (i <- 1 to attempts ; p <- systemProperties)
+      p match { case (k, v) => assert (k != null && v != null, fail(i)) }
+  }
+  probe onComplete {
+    case _ => done = true
+  }
+
+  System.setProperty("foo", "fooz")
+  System.setProperty("bar", "barz")
+  barrier.await()   // just for fun, wait to start mucking with properties
+
+  // continually modify properties trying to break live iteration over sys props
+  // hint: don't iterate lively over sys props
+  var alt = true
+  while (!done) {
+    if (alt) {
+      System.getProperties.remove("foo")
+      System.setProperty("bar", "barz")
+      alt = false
+    } else {
+      System.getProperties.remove("bar")
+      System.setProperty("foo", "fooz")
+      alt = true
+    }
+  }
+  Await.result(probe, Duration.Inf)
+}
+
+/*
 import scala.concurrent.{duration, Future, Await, ExecutionContext}
 import scala.tools.nsc.Settings
 import ExecutionContext.Implicits.global
@@ -15,3 +57,4 @@ object Test {
     Await.result(compiler, duration.Duration.Inf)
   }
 }
+*/