Bug 1612403 - File extensions are duplicated for file inputs with cus…

…tom 'accept'. r=Gijs

Differential Revision: https://phabricator.services.mozilla.com/D63071

uriloader/exthandler/HandlerServiceParent.cpp

@@ -170,13 +170,17 @@ NS_IMETHODIMP ProxyMIMEInfo::SetFileExtensions(const nsACString& aExtensions) {
 /* boolean extensionExists (in AUTF8String aExtension); */
 NS_IMETHODIMP ProxyMIMEInfo::ExtensionExists(const nsACString& aExtension,
                                              bool* _retval) {
-  *_retval = mProxyHandlerInfo->Extensions().Contains(aExtension);
+  *_retval = mProxyHandlerInfo->Extensions().Contains(
+      aExtension, nsCaseInsensitiveCStringArrayComparator());
   return NS_OK;
 }
 
 /* void appendExtension (in AUTF8String aExtension); */
 NS_IMETHODIMP ProxyMIMEInfo::AppendExtension(const nsACString& aExtension) {
-  mProxyHandlerInfo->Extensions().AppendElement(aExtension);
+  if (!mProxyHandlerInfo->Extensions().Contains(
+          aExtension, nsCaseInsensitiveCStringArrayComparator())) {
+    mProxyHandlerInfo->Extensions().AppendElement(aExtension);
+  }
   return NS_OK;
 }
 

uriloader/exthandler/android/nsMIMEInfoAndroid.cpp

@@ -238,15 +238,16 @@ nsMIMEInfoAndroid::GetFileExtensions(nsIUTF8StringEnumerator** aResult) {
 NS_IMETHODIMP
 nsMIMEInfoAndroid::SetFileExtensions(const nsACString& aExtensions) {
   mExtensions.Clear();
-  nsCString extList(aExtensions);
-
-  int32_t breakLocation = -1;
-  while ((breakLocation = extList.FindChar(',')) != -1) {
-    mExtensions.AppendElement(
-        Substring(extList.get(), extList.get() + breakLocation));
-    extList.Cut(0, breakLocation + 1);
+  nsACString::const_iterator start, end;
+  aExtensions.BeginReading(start);
+  aExtensions.EndReading(end);
+  while (start != end) {
+    nsACString::const_iterator cursor = start;
+    mozilla::Unused << FindCharInReadable(',', cursor, end);
+    AddUniqueExtension(Substring(start, cursor));
+    // If a comma was found, skip it for the next search.
+    start = cursor != end ? ++cursor : cursor;
   }
-  if (!extList.IsEmpty()) mExtensions.AppendElement(extList);
   return NS_OK;
 }
 
@@ -268,9 +269,18 @@ nsMIMEInfoAndroid::ExtensionExists(const nsACString& aExtension,
   return NS_OK;
 }
 
+void nsMIMEInfoAndroid::AddUniqueExtension(const nsACString& aExtension) {
+  MOZ_ASSERT(!aExtension.IsEmpty(), "no extension");
+  if (!mExtensions.Contains(aExtension,
+                            nsCaseInsensitiveCStringArrayComparator())) {
+    mExtensions.AppendElement(aExtension);
+  }
+}
+
 NS_IMETHODIMP
 nsMIMEInfoAndroid::AppendExtension(const nsACString& aExtension) {
-  mExtensions.AppendElement(aExtension);
+  MOZ_ASSERT(!aExtension.IsEmpty(), "No extension");
+  AddUniqueExtension(aExtension);
   return NS_OK;
 }
 
@@ -284,22 +294,13 @@ nsMIMEInfoAndroid::GetPrimaryExtension(nsACString& aPrimaryExtension) {
 
 NS_IMETHODIMP
 nsMIMEInfoAndroid::SetPrimaryExtension(const nsACString& aExtension) {
-  uint32_t extCount = mExtensions.Length();
-  uint8_t i;
-  bool found = false;
-  for (i = 0; i < extCount; i++) {
-    const nsCString& ext = mExtensions[i];
-    if (ext.Equals(aExtension, nsCaseInsensitiveCStringComparator())) {
-      found = true;
-      break;
-    }
-  }
-  if (found) {
+  MOZ_ASSERT(!aExtension.IsEmpty(), "No extension");
+  int32_t i = mExtensions.IndexOf(aExtension, 0,
+                                  nsCaseInsensitiveCStringArrayComparator());
+  if (i != -1) {
     mExtensions.RemoveElementAt(i);
   }
-
   mExtensions.InsertElementAt(0, aExtension);
-
   return NS_OK;
 }
 

uriloader/exthandler/android/nsMIMEInfoAndroid.h

@@ -50,6 +50,11 @@ class nsMIMEInfoAndroid final : public nsIMIMEInfo {
    private:
     ~SystemChooser() {}
 
+    /**
+     * Internal helper to avoid adding duplicates.
+     */
+    void AddUniqueExtension(const nsACString& aExtension);
+
     nsMIMEInfoAndroid* mOuter;
   };
 };

uriloader/exthandler/nsMIMEInfoImpl.cpp

@@ -95,20 +95,9 @@ nsMIMEInfoBase::GetFileExtensions(nsIUTF8StringEnumerator** aResult) {
 
 NS_IMETHODIMP
 nsMIMEInfoBase::ExtensionExists(const nsACString& aExtension, bool* _retval) {
-  NS_ASSERTION(!aExtension.IsEmpty(), "no extension");
-  bool found = false;
-  uint32_t extCount = mExtensions.Length();
-  if (extCount < 1) return NS_OK;
-
-  for (uint8_t i = 0; i < extCount; i++) {
-    const nsCString& ext = mExtensions[i];
-    if (ext.Equals(aExtension, nsCaseInsensitiveCStringComparator())) {
-      found = true;
-      break;
-    }
-  }
-
-  *_retval = found;
+  MOZ_ASSERT(!aExtension.IsEmpty(), "no extension");
+  *_retval = mExtensions.Contains(aExtension,
+                                  nsCaseInsensitiveCStringArrayComparator());
   return NS_OK;
 }
 
@@ -122,29 +111,28 @@ nsMIMEInfoBase::GetPrimaryExtension(nsACString& _retval) {
 
 NS_IMETHODIMP
 nsMIMEInfoBase::SetPrimaryExtension(const nsACString& aExtension) {
-  NS_ASSERTION(!aExtension.IsEmpty(), "no extension");
-  uint32_t extCount = mExtensions.Length();
-  uint8_t i;
-  bool found = false;
-  for (i = 0; i < extCount; i++) {
-    const nsCString& ext = mExtensions[i];
-    if (ext.Equals(aExtension, nsCaseInsensitiveCStringComparator())) {
-      found = true;
-      break;
-    }
-  }
-  if (found) {
+  MOZ_ASSERT(!aExtension.IsEmpty(), "No extension");
+  int32_t i = mExtensions.IndexOf(aExtension, 0,
+                                  nsCaseInsensitiveCStringArrayComparator());
+  if (i != -1) {
     mExtensions.RemoveElementAt(i);
   }
-
   mExtensions.InsertElementAt(0, aExtension);
-
   return NS_OK;
 }
 
+void nsMIMEInfoBase::AddUniqueExtension(const nsACString& aExtension) {
+  if (!aExtension.IsEmpty() &&
+      !mExtensions.Contains(aExtension,
+                            nsCaseInsensitiveCStringArrayComparator())) {
+    mExtensions.AppendElement(aExtension);
+  }
+}
+
 NS_IMETHODIMP
 nsMIMEInfoBase::AppendExtension(const nsACString& aExtension) {
-  mExtensions.AppendElement(aExtension);
+  MOZ_ASSERT(!aExtension.IsEmpty(), "No extension");
+  AddUniqueExtension(aExtension);
   return NS_OK;
 }
 
@@ -192,15 +180,16 @@ nsMIMEInfoBase::Equals(nsIMIMEInfo* aMIMEInfo, bool* _retval) {
 NS_IMETHODIMP
 nsMIMEInfoBase::SetFileExtensions(const nsACString& aExtensions) {
   mExtensions.Clear();
-  nsCString extList(aExtensions);
-
-  int32_t breakLocation = -1;
-  while ((breakLocation = extList.FindChar(',')) != -1) {
-    mExtensions.AppendElement(
-        Substring(extList.get(), extList.get() + breakLocation));
-    extList.Cut(0, breakLocation + 1);
+  nsACString::const_iterator start, end;
+  aExtensions.BeginReading(start);
+  aExtensions.EndReading(end);
+  while (start != end) {
+    nsACString::const_iterator cursor = start;
+    mozilla::Unused << FindCharInReadable(',', cursor, end);
+    AddUniqueExtension(Substring(start, cursor));
+    // If a comma was found, skip it for the next search.
+    start = cursor != end ? ++cursor : cursor;
   }
-  if (!extList.IsEmpty()) mExtensions.AppendElement(extList);
   return NS_OK;
 }
 

uriloader/exthandler/nsMIMEInfoImpl.h

@@ -137,6 +137,11 @@ class nsMIMEInfoBase : public nsIMIMEInfo {
    */
   static nsresult GetLocalFileFromURI(nsIURI* aURI, nsIFile** aFile);
 
+  /**
+   * Internal helper to avoid adding duplicates.
+   */
+  void AddUniqueExtension(const nsACString& aExtension);
+
   // member variables
   nsTArray<nsCString>
       mExtensions;  ///< array of file extensions associated w/ this MIME obj

uriloader/exthandler/tests/unit/test_handlerService_store.js

@@ -53,8 +53,7 @@ function assertAllHandlerInfosMatchTestData() {
 
   // It's important that the MIME types we check here do not exist at the
   // operating system level, otherwise the list of handlers and file extensions
-  // will be merged. The current implementation adds each saved file extension
-  // even if one already exists in the system, resulting in duplicate entries.
+  // will be merged. The current implementation avoids duplicate entries.
 
   HandlerServiceTestUtils.assertHandlerInfoMatches(handlerInfos.shift(), {
     type: "example/type.handleinternally",
@@ -415,8 +414,8 @@ add_task(async function test_store_fileExtensions_lowercase() {
 });
 
 /**
- * Tests that duplicates added with "appendExtension" or present in
- * "possibleApplicationHandlers" are removed when saving and reloading.
+ * Tests that appendExtension doesn't add duplicates, and that anyway duplicates
+ * from possibleApplicationHandlers are removed when saving and reloading.
  */
 add_task(async function test_store_no_duplicates() {
   await deleteHandlerStore();
@@ -431,6 +430,10 @@ add_task(async function test_store_no_duplicates() {
   handlerInfo.appendExtension("extension_test2");
   handlerInfo.appendExtension("extension_test1");
   handlerInfo.appendExtension("EXTENSION_test1");
+  Assert.deepEqual(Array.from(handlerInfo.getFileExtensions()), [
+    "extension_test1",
+    "extension_test2",
+  ]);
   gHandlerService.store(handlerInfo);
 
   await unloadHandlerStore();
@@ -449,6 +452,21 @@ add_task(async function test_store_no_duplicates() {
   });
 });
 
+/**
+ * Tests that setFileExtensions doesn't add duplicates.
+ */
+add_task(async function test_setFileExtensions_no_duplicates() {
+  await deleteHandlerStore();
+
+  let handlerInfo = getKnownHandlerInfo("example/new");
+  handlerInfo.setFileExtensions("a,b,A,b,c,a");
+  let expected = ["a", "b", "c"];
+  Assert.deepEqual(Array.from(handlerInfo.getFileExtensions()), expected);
+  // Test empty extensions, also at begin and end.
+  handlerInfo.setFileExtensions(",a,,b,A,c,");
+  Assert.deepEqual(Array.from(handlerInfo.getFileExtensions()), expected);
+});
+
 /**
  * Tests that "store" deletes properties that have their default values from
  * the data store.