SharedPreferencesManager#readStringSet() returns an unmodifiable Set.

Modifying the set returned by SharedPreferences#getStringSet() is
forbidden by the API, but nothing stops the developer from doing so.
It can cause consistency issues resulting in loss of data.

Change-Id: I84907d4c4475d2a97974a6b57e16c50a411d915c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2039811
Commit-Queue: Henrique Nakashima <hnakashima@chromium.org>
Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Reviewed-by: Natalie Chouinard <chouinard@chromium.org>
Cr-Commit-Position: refs/heads/master@{#738677}

chrome/browser/preferences/android/java/src/org/chromium/chrome/browser/preferences/SharedPreferencesManager.java

@@ -106,7 +106,9 @@ public class SharedPreferencesManager {
     /**
      * Reads set of String values from preferences.
      *
-     * Note that you must not modify the set instance returned by this call.
+     * If no value was set for the |key|, returns an unmodifiable empty set.
+     *
+     * @return unmodifiable Set with the values
      */
     public Set<String> readStringSet(String key) {
         return readStringSet(key, Collections.emptySet());
@@ -115,11 +117,15 @@ public class SharedPreferencesManager {
     /**
      * Reads set of String values from preferences.
      *
-     * Note that you must not modify the set instance returned by this call.
+     * If no value was set for the |key|, returns an unmodifiable view of |defaultValue|.
+     *
+     * @return unmodifiable Set with the values
      */
-    public Set<String> readStringSet(String key, Set<String> defaultValue) {
+    @Nullable
+    public Set<String> readStringSet(String key, @Nullable Set<String> defaultValue) {
         mKeyChecker.checkIsKeyInUse(key);
-        return ContextUtils.getAppSharedPreferences().getStringSet(key, defaultValue);
+        Set<String> values = ContextUtils.getAppSharedPreferences().getStringSet(key, defaultValue);
+        return (values != null) ? Collections.unmodifiableSet(values) : null;
     }
 
     /**
@@ -417,6 +423,7 @@ public class SharedPreferencesManager {
      * @param defaultValue The default value to return if there's no value stored.
      * @return The value of the preference if stored; defaultValue otherwise.
      */
+    @Nullable
     public String readString(String key, @Nullable String defaultValue) {
         mKeyChecker.checkIsKeyInUse(key);
         try (StrictModeContext ignored = StrictModeContext.allowDiskReads()) {

chrome/browser/preferences/android/java/src/org/chromium/chrome/browser/preferences/SharedPreferencesManagerTest.java

@@ -244,6 +244,18 @@ public class SharedPreferencesManagerTest {
         assertEquals(Collections.emptySet(), mSubject.readStringSet("string_set_key"));
     }
 
+    @Test(expected = UnsupportedOperationException.class)
+    @SmallTest
+    public void testReadStringSet_nonEmpty_returnsUnmodifiable() {
+        Set<String> exampleStringSet = new HashSet<>(Arrays.asList("d", "e"));
+        mSubject.writeStringSet("string_set_key", exampleStringSet);
+
+        Set<String> unmodifiableSet = mSubject.readStringSet("string_set_key");
+
+        // Should throw an exception
+        unmodifiableSet.add("f");
+    }
+
     @Test
     @SmallTest
     public void testWriteIntSync() {