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

lib/options.js

Index: lib/options.js
diff --git a/lib/options.js b/lib/options.js
index fa39d78374b04df576ddc85aedd5aea613fffe39..7e4d4909b1100183ae5139b98470f35756b0d0f3 100644
--- a/lib/options.js
+++ b/lib/options.js
@@ -38,7 +38,51 @@ function findOptionsTab(callback)
     // 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);
-    callback(tabs.find(element => element.url == fullOptionsUrl));
+    let optionsTab = tabs.find(tab => tab.url == fullOptionsUrl);
+
+    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
+    {
+      // 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.

+      {
+        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.

+        {
+          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);
+      }
+      else
+        callback();
+    }
   });
 }