Bug 1339782 - use a WeakMap to keep track of windows with active findbar highlighters and make sure no JS error occurs when a window is closed. r?jaws draft
authorMike de Boer <mdeboer@mozilla.com>
Mon, 27 Feb 2017 16:15:12 +0100
changeset 490026 8fdde077b66f8f77ade1cce7ec742f8c85523ced
parent 490024 cd0c8efc455a7b09be29d1ed33eaa3b090b56865
child 547144 2e167c7331752b0f0209885760400abfe4b6129f
push id46973
push usermdeboer@mozilla.com
push dateMon, 27 Feb 2017 15:17:06 +0000
reviewersjaws
bugs1339782
milestone54.0a1
Bug 1339782 - use a WeakMap to keep track of windows with active findbar highlighters and make sure no JS error occurs when a window is closed. r?jaws MozReview-Commit-ID: KGdvuWJZzMV
toolkit/modules/Finder.jsm
toolkit/modules/FinderHighlighter.jsm
--- a/toolkit/modules/Finder.jsm
+++ b/toolkit/modules/Finder.jsm
@@ -52,22 +52,23 @@ Finder.prototype = {
       return this._iterator;
     this._iterator = Cu.import("resource://gre/modules/FinderIterator.jsm", null).FinderIterator;
     return this._iterator;
   },
 
   destroy() {
     if (this._iterator)
       this._iterator.reset();
-    if (this._highlighter) {
+    let window = this._getWindow();
+    if (this._highlighter && window) {
       // if we clear all the references before we hide the highlights (in both
       // highlighting modes), we simply can't use them to find the ranges we
       // need to clear from the selection.
-      this._highlighter.hide();
-      this._highlighter.clear();
+      this._highlighter.hide(window);
+      this._highlighter.clear(window);
     }
     this.listeners = [];
     this._docShell.QueryInterface(Ci.nsIInterfaceRequestor)
       .getInterface(Ci.nsIWebProgress)
       .removeProgressListener(this, Ci.nsIWebProgress.NOTIFY_LOCATION);
     this._listeners = [];
     this._currentFoundRange = this._fastFind = this._docShell = this._previousLink =
       this._highlighter = null;
@@ -291,17 +292,17 @@ Finder.prototype = {
   enableSelection() {
     this._fastFind.setSelectionModeAndRepaint(Ci.nsISelectionController.SELECTION_ON);
     this._restoreOriginalOutline();
   },
 
   removeSelection() {
     this._fastFind.collapseSelection();
     this.enableSelection();
-    this.highlighter.clear();
+    this.highlighter.clear(this._getWindow());
   },
 
   focusContent() {
     // Allow Finder listeners to cancel focusing the content.
     for (let l of this._listeners) {
       try {
         if ("shouldFocusContent" in l &&
             !l.shouldFocusContent())
@@ -470,16 +471,18 @@ Finder.prototype = {
     this._currentMatchesCountResult = {
       total: 0,
       current: 0,
       _currentFound: false
     };
   },
 
   _getWindow() {
+    if (!this._docShell)
+      return null;
     return this._docShell.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindow);
   },
 
   /**
    * Get the bounding selection rect in CSS px relative to the origin of the
    * top-level content document.
    */
   _getResultRect() {
--- a/toolkit/modules/FinderHighlighter.jsm
+++ b/toolkit/modules/FinderHighlighter.jsm
@@ -126,17 +126,17 @@ function mockAnonymousContentNode(domNod
       return (domNode.querySelector("#" + id) || domNode).animate(keyframes, duration);
     },
     setCutoutRectsForElement(id, rects) {
       // no-op for now.
     }
   };
 }
 
-let gWindows = new Map();
+let gWindows = new WeakMap();
 
 /**
  * FinderHighlighter class that is used by Finder.jsm to take care of the
  * 'Highlight All' feature, which can highlight all find occurrences in a page.
  *
  * @param {Finder} finder Finder.jsm instance
  */
 function FinderHighlighter(finder) {
@@ -332,19 +332,19 @@ FinderHighlighter.prototype = {
    *
    * @param {nsIDOMWindow} window    The dimmed background will overlay this window.
    *                                 Optional, defaults to the finder window.
    * @param {nsIDOMRange}  skipRange A range that should not be removed from the
    *                                 find selection.
    * @param {nsIDOMEvent}  event     When called from an event handler, this will
    *                                 be the triggering event.
    */
-  hide(window = null, skipRange = null, event = null) {
+  hide(window, skipRange = null, event = null) {
     try {
-      window = (window || this.finder._getWindow()).top;
+      window = window.top;
     } catch (ex) {
       Cu.reportError(ex);
       return;
     }
     let dict = this.getForWindow(window);
 
     let isBusySelecting = dict.busySelecting;
     dict.busySelecting = false;
@@ -407,17 +407,17 @@ FinderHighlighter.prototype = {
    *                           by the consumer of the Finder.
    */
   update(data) {
     let window = this.finder._getWindow();
     let dict = this.getForWindow(window);
     let foundRange = this.finder._fastFind.getFoundRange();
 
     if (data.result == Ci.nsITypeAheadFind.FIND_NOTFOUND || !data.searchString || !foundRange) {
-      this.hide();
+      this.hide(window);
       return;
     }
 
     if (!this._modal) {
       if (this._highlightAll) {
         dict.previousFoundRange = dict.currentFoundRange;
         dict.currentFoundRange = foundRange;
         let params = this.iterator.params;
@@ -449,24 +449,18 @@ FinderHighlighter.prototype = {
       this.highlight(true, data.searchString, data.linksOnly);
   },
 
   /**
    * Invalidates the list by clearing the map of highlighted ranges that we
    * keep to build the mask for.
    */
   clear(window = null) {
-    if (!window) {
-      // Since we're clearing _all the things_, make sure we hide 'em all as well.
-      for (let win of gWindows.keys())
-        this.hide(win);
-      // Reset the Map, because no range references a node anymore.
-      gWindows.clear();
+    if (!window || !window.top)
       return;
-    }
 
     let dict = this.getForWindow(window.top);
     this._finishOutlineAnimations(dict);
     dict.dynamicRangesSet.clear();
     dict.frames.clear();
     dict.modalHighlightRectsMap.clear();
     dict.brightText = null;
   },
@@ -474,34 +468,37 @@ FinderHighlighter.prototype = {
   /**
    * When the current page is refreshed or navigated away from, the CanvasFrame
    * contents is not valid anymore, i.e. all anonymous content is destroyed.
    * We need to clear the references we keep, which'll make sure we redraw
    * everything when the user starts to find in page again.
    */
   onLocationChange() {
     let window = this.finder._getWindow();
+    if (!window || !window.top)
+      return;
     this.hide(window);
     this.clear(window);
     this._removeRangeOutline(window);
 
     gWindows.delete(window.top);
   },
 
   /**
    * When `kModalHighlightPref` pref changed during a session, this callback is
    * invoked. When modal highlighting is turned off, we hide the CanvasFrame
    * contents.
    *
    * @param {Boolean} useModalHighlight
    */
   onModalHighlightChange(useModalHighlight) {
-    if (this._modal && !useModalHighlight) {
-      this.hide();
-      this.clear();
+    let window = this.finder._getWindow();
+    if (window && this._modal && !useModalHighlight) {
+      this.hide(window);
+      this.clear(window);
     }
     this._modal = useModalHighlight;
   },
 
   /**
    * When 'Highlight All' is toggled during a session, this callback is invoked
    * and when it's turned off, the found occurrences will be removed from the mask.
    *