Issue 5965 - Handle edge case when searching for options page tab

lib/options.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 21 matching lines @@
   {
     // We find a tab ourselves because Edge has a bug when quering tabs with
     // extension URL protocol:
     // https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/8094141/
     // https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/8604703/
     // Firefox won't let us query for moz-extension:// pages either, though
     // starting with Firefox 56 an extension can query for its own URLs:
     // https://bugzilla.mozilla.org/show_bug.cgi?id=1271354
     let fullOptionsUrl = browser.extension.getURL(optionsUrl);
     let optionsTab = tabs.find(tab => tab.url == fullOptionsUrl);
+    if (optionsTab)
+    {
+      callback(optionsTab);
+      return;
+    }
 
-    if (optionsTab)
-      callback(optionsTab);

[Wladimir Palant, 2017/11/11 18:34:14]

Maybe reduce the indentation level here:

  if (optionsTab)
  {
    callback(optionsTab);
    return;
  }

[kzar, 2017/11/13 12:32:57]

Done.

-    else
+    // Newly created tabs might have about:blank as their URL in Firefox rather
+    // than the final options page URL, we need to wait for those to finish
+    // loading.
+    let potentialOptionTabIds = new Set(
+      tabs.filter(tab => tab.url == "about:blank" && tab.status == "loading")
+          .map(tab => tab.id)
+    );
+    if (potentialOptionTabIds.size == 0)
     {
-      // It seems that the url property isn't always set for the tab with
-      // Firefox 57 on Windows 10 until the tab has finished loading. Until
-      // then the url is given as "about:blank", but the title is the URL
-      // we expect (minus the protocol part).

[Wladimir Palant, 2017/11/11 18:34:14]

That's too many details IMHO. We don't really know which Firefox versions and
which operating systems are affected. All we know is that there are conditions
where we might get a tab that isn't fully initialized yet. So maybe something
like: "Newly created tabs might have about:blank as their URL in Firefox rather
than the final options page URL, we need to wait for those to finish loading."

[kzar, 2017/11/13 12:32:57]

Done.

-      let {hostname, pathname} = new URL(fullOptionsUrl);
-      let potentialOptionsTab = tabs.find(
-        tab => tab.url == "about:blank" &&
-               tab.title == hostname + pathname &&

[Wladimir Palant, 2017/11/11 18:34:14]

I don't think that we should rely on the tab title. We should just watch all
about:blank tabs that are still loading, these are potentially our options tab.

[kzar, 2017/11/13 12:32:57]

Done.

-               tab.status == "loading"
-      );
-
-      if (potentialOptionsTab)

[Wladimir Palant, 2017/11/11 18:34:14]

Maybe reduce indentation level here as well:

  if (!potentialOptionsTabs.length)
  {
    callback();
    return;
  }

[kzar, 2017/11/13 12:32:57]

Done.

+      callback();
+      return;
+    }
+    let removeListener;
+    let updateListener = (tabId, changeInfo, tab) =>
+    {
+      if (potentialOptionTabIds.has(tabId) &&
+          changeInfo.status == "complete")
       {
-        let onRemoved;
-        let updateListener = (tabId, changeInfo, tab) =>

[Wladimir Palant, 2017/11/11 18:34:14]

What about consistent naming? Meaning either removeListener or onUpdated.

[kzar, 2017/11/13 12:32:57]

Done.

+        potentialOptionTabIds.delete(tabId);
+        let urlMatch = tab.url == fullOptionsUrl;
+        if (urlMatch || potentialOptionTabIds.size == 0)
         {
-          if (tabId == potentialOptionsTab.id &&
-              changeInfo.status == "complete")
-          {
-            browser.tabs.onUpdated.removeListener(updateListener);
-            browser.tabs.onRemoved.removeListener(onRemoved);
-            callback(tab.url == fullOptionsUrl ? tab : undefined);
-          }
-        };
-        browser.tabs.onUpdated.addListener(updateListener);
-        onRemoved = removedTabId =>
-        {
-          if (removedTabId == potentialOptionsTab.id)
-          {
-            browser.tabs.onUpdated.removeListener(updateListener);
-            browser.tabs.onRemoved.removeListener(onRemoved);
-            callback();
-          }
-        };
-        browser.tabs.onRemoved.addListener(onRemoved);
+          browser.tabs.onUpdated.removeListener(updateListener);
+          browser.tabs.onRemoved.removeListener(removeListener);
+          callback(urlMatch ? tab : undefined);
+        }
       }
-      else
+    };
+    browser.tabs.onUpdated.addListener(updateListener);
+    removeListener = removedTabId =>
+    {
+      potentialOptionTabIds.delete(removedTabId);
+      if (potentialOptionTabIds.size == 0)
+      {
+        browser.tabs.onUpdated.removeListener(updateListener);
+        browser.tabs.onRemoved.removeListener(removeListener);
         callback();
-    }
+      }
+    };
+    browser.tabs.onRemoved.addListener(removeListener);
   });
 }
 
 function returnShowOptionsCall(optionsTab, callback)
 {
   if (!callback)
     return;
 
   if (optionsTab)
   {
@@ skipping 91 matching lines @@
         args: [
           {
             host: getDecodedHostname(currentPage.url).replace(/^www\./, ""),
             whitelisted: !!checkWhitelisted(currentPage)
           }
         ]
       });
     });
   });
 });