Better close-down for global link hints.

- Unregister frames with the background-page hint coordinator.
- Better page cleanup in the content scripts.
- Require documentReady before harvesting hints.

There is still an issue in some cases when link-hints are activated as a
page is transitioning, but that problem case seems to have existed in
1.54.  I'll continue to investigate.

3 files changed, 33 insertions, 48 deletions

diff --git a/background_scripts/main.coffee b/background_scripts/main.coffee
index 6dd3906f..addbf514 100644
--- a/background_scripts/main.coffee
+++ b/background_scripts/main.coffee
@@ -300,11 +300,12 @@ Frames =
     frameIdsForTab[tabId].push frameId unless frameId in frameIdsForTab[tabId] ?= []
     (portsForTab[tabId] ?= {})[frameId] = port
 
-  unregsterFrame: ({tabId, frameId}) ->
+  unregisterFrame: ({tabId, frameId}) ->
     if tabId of frameIdsForTab
       frameIdsForTab[tabId] = (fId for fId in frameIdsForTab[tabId] when fId != frameId)
     if tabId of portsForTab
       delete portsForTab[tabId][frameId]
+    HintCoordinator.unregisterFrame tabId, frameId
 
   isEnabledForUrl: ({request, tabId, port}) ->
     urlForTab[tabId] = request.url if request.frameIsFocused
@@ -345,11 +346,9 @@ cycleToFrame = (frames, frameId, count = 0) ->
   [frames[count..]..., frames[0...count]...]
 
 HintCoordinator =
-  debug: false
   tabState: {}
 
   onMessage: (tabId, frameId, request) ->
-    console.log "onMessage", tabId, frameId, "[#{request.messageType}]" if @debug
     if request.messageType of this
       this[request.messageType] tabId, frameId, request
     else
@@ -357,46 +356,31 @@ HintCoordinator =
       @sendMessage request.messageType, tabId, request
 
   sendMessage: (messageType, tabId, request = {}) ->
-    console.log "sendMessage", tabId, "[#{messageType}] [#{@tabState[tabId].ports.length}]" if @debug
     extend request, {handler: "linkHintsMessage", messageType}
-    for own frameId, port of @tabState[tabId].ports
-      try
-        port.postMessage request
-      catch
-        # A frame has gone away; remove it from consideration.
-        delete @tabState[tabId].ports[frameId]
-        # It could be that we're expecting hints from this frame; schedule "sending" dummy/empty hints instead.
-        Utils.nextTick =>
-          @postHintDescriptors tabId, frameId, hintDescriptors: []
-    # We can delete the tab state when we see an "exit" message, that's the last message in the sequence.
-    delete @tabState[tabId] if messageType == "exit"
+    port.postMessage request for own _, port of @tabState[tabId].ports
 
   prepareToActivateMode: (tabId, originatingFrameId, {modeIndex}) ->
-    console.log "" if @debug
-    console.log "prepareToActivateMode", tabId, "[#{frameIdsForTab[tabId].length}]" if @debug
     @tabState[tabId] = {frameIds: frameIdsForTab[tabId][..], hintDescriptors: [], originatingFrameId, modeIndex}
     @tabState[tabId].ports = extend {}, portsForTab[tabId]
     @sendMessage "getHintDescriptors", tabId, {modeIndex}
-    # FIXME(smblott)  This should not be necessary; it's a backstop to mitigate against the possibility that,
-    # for some reason, we do not hear back from a frame.
-    unless @debug
-      for frameId in frameIdsForTab[tabId]
-        do (frameId) =>
-          Utils.setTimeout 400, =>
-            @postHintDescriptors tabId, frameId, hintDescriptors: []
 
   # Receive hint descriptors from all frames and activate link-hints mode when we have them all.
   postHintDescriptors: (tabId, frameId, {hintDescriptors}) ->
     if frameId in @tabState[tabId].frameIds
       @tabState[tabId].hintDescriptors.push hintDescriptors...
       @tabState[tabId].frameIds = @tabState[tabId].frameIds.filter (fId) -> fId != frameId
-      console.log "postHintDescriptors", tabId, frameId, "[#{@tabState[tabId].frameIds.length}]" if @debug
       if @tabState[tabId].frameIds.length == 0
         @sendMessage "activateMode", tabId,
           originatingFrameId: @tabState[tabId].originatingFrameId
           hintDescriptors: @tabState[tabId].hintDescriptors
           modeIndex: @tabState[tabId].modeIndex
 
+  # If an unregistering frame is participating in link-hints mode, then we need to tidy up after it.
+  unregisterFrame: (tabId, frameId) ->
+    delete @tabState[tabId]?.ports?[frameId]
+    # We fake "postHintDescriptors" for an unregistering frame, if necessary.
+    @postHintDescriptors tabId, frameId, hintDescriptors: [] if @tabState[tabId]?.frameIds
+
 # Port handler mapping
 portHandlers =
   completions: handleCompletions
diff --git a/content_scripts/link_hints.coffee b/content_scripts/link_hints.coffee
index e650463f..56cea78d 100644
--- a/content_scripts/link_hints.coffee
+++ b/content_scripts/link_hints.coffee
@@ -50,29 +50,25 @@ availableModes = [OPEN_IN_CURRENT_TAB, OPEN_IN_NEW_BG_TAB, OPEN_IN_NEW_FG_TAB, O
   OPEN_INCOGNITO, DOWNLOAD_LINK_URL]
 
 HintCoordinator =
-  debug: false
   onExit: []
   localHints: null
   suppressKeyboardEvents: null
 
   sendMessage: (messageType, request = {}) ->
-    console.log "sendMessage", frameId, "[#{messageType}]" if @debug
     Frame.postMessage "linkHintsMessage", extend request, {messageType}
 
   prepareToActivateMode: (mode, onExit) ->
-    console.log "prepareToActivateMode", frameId if @debug
     # We need to communicate with the background page (and other frames) to initiate link-hints mode.  To
     # prevent other Vimium commands from being triggered before link-hints mode is launched, we install a
     # temporary mode to block keyboard events.
-    @suppressKeyboardEvents = new SuppressAllKeyboardEvents
+    @suppressKeyboardEvents = suppressKeyboardEvents = new SuppressAllKeyboardEvents
       name: "link-hints/suppress-keyboard-events"
       singleton: "link-hints-mode"
       indicator: "Collecting hints..."
       exitOnEscape: true
     # FIXME(smblott) Global link hints is currently insufficiently reliable.  If the mode above is left in
     # place, then Vimium blocks.  As a temporary measure, we install a timer to remove it.
-    unless @debug
-      Utils.setTimeout 1000, => @suppressKeyboardEvents.exit() if @suppressKeyboardEvents?.modeIsActive
+    Utils.setTimeout 1000, -> suppressKeyboardEvents.exit() if suppressKeyboardEvents?.modeIsActive
     @onExit = [onExit]
     @sendMessage "prepareToActivateMode", modeIndex: availableModes.indexOf mode
 
@@ -82,12 +78,10 @@ HintCoordinator =
   #   localIndex: the index in @localHints for the full hint descriptor for this hint
   #   linkText: the link's text for filtered hints (this is null for alphabet hints)
   getHintDescriptors: ({modeIndex}) ->
-    console.log "getHintDescriptors", frameId if @debug
-    # Ensure that the settings are loaded.  The request might have been initiated in another frame.
-    Settings.onLoaded =>
+    # Ensure that the document is ready and that the settings are loaded.
+    DomUtils.documentReady => Settings.onLoaded =>
       requireHref = availableModes[modeIndex] in [COPY_LINK_URL, OPEN_INCOGNITO]
       @localHints = LocalHints.getLocalHints requireHref
-      console.log "getHintDescriptors", frameId, "[#{@localHints.length}]" if @debug
       @sendMessage "postHintDescriptors", hintDescriptors:
         @localHints.map ({linkText}, localIndex) -> {frameId, localIndex, linkText}
 
@@ -95,11 +89,10 @@ HintCoordinator =
   # We also propagate the key state between frames.  Therefore, the hint-selection process proceeds in lock
   # step in every frame, and @linkHintsMode is in the same state in every frame.
   activateMode: ({hintDescriptors, modeIndex, originatingFrameId}) ->
-    console.log "activateMode", frameId if @debug
-    @suppressKeyboardEvents?.exit() if @suppressKeyboardEvents?.modeIsActive
-    @suppressKeyboardEvents = null
-    # Ensure that the settings are loaded.  The request might have been initiated in another frame.
-    Settings.onLoaded =>
+    # Ensure that the document is ready and that the settings are loaded.
+    DomUtils.documentReady => Settings.onLoaded =>
+      @suppressKeyboardEvents.exit() if @suppressKeyboardEvents?.modeIsActive
+      @suppressKeyboardEvents = null
       @onExit = [] unless frameId == originatingFrameId
       @linkHintsMode = new LinkHintsMode hintDescriptors, availableModes[modeIndex]
 
@@ -110,8 +103,7 @@ HintCoordinator =
   getLocalHintMarker: (hint) -> if hint.frameId == frameId then @localHints[hint.localIndex] else null
 
   exit: ({isSuccess}) ->
-    console.log "exit", frameId, "[#{isSuccess}]" if @debug
-    @linkHintsMode.deactivateMode()
+    @linkHintsMode?.deactivateMode()
     @onExit.pop() isSuccess while 0 < @onExit.length
     @linkHintsMode = @localHints = null
 
diff --git a/content_scripts/vimium_frontend.coffee b/content_scripts/vimium_frontend.coffee
index d9fc363e..58cf6115 100644
--- a/content_scripts/vimium_frontend.coffee
+++ b/content_scripts/vimium_frontend.coffee
@@ -239,12 +239,21 @@ Frame =
     @port.onMessage.addListener (request) =>
       (@listeners[request.handler] ? this[request.handler]) request
 
-    @port.onDisconnect.addListener ->
-      # We disable the content scripts when we lose contact with the background page.
-      isEnabledForUrl = false
-      window.removeEventListener "focus", onFocus
-
-    window.addEventListener "unload", => @postMessage "unregsterFrame"
+    # We disable the content scripts when we lose contact with the background page, or on unload.
+    @port.onDisconnect.addListener disconnect = Utils.makeIdempotent => @disconnect()
+    window.addEventListener "unload", disconnect
+
+  disconnect: ->
+    try @postMessage "unregisterFrame"
+    try @port.disconnect()
+    @postMessage = @disconnect = ->
+    @port = null
+    @listeners = {}
+    HintCoordinator.exit isSuccess: false
+    handlerStack.reset()
+    isEnabledForUrl = false
+    window.removeEventListener "focus", onFocus
+    window.removeEventListener "hashchange", onFocus
 
 setScrollPosition = ({ scrollX, scrollY }) ->
   if DomUtils.isTopFrame()