749738 - Findbar triggers multiple dead object exceptions (can't close findbar)

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Ed Lee :Mardak]

In _getSelectionController:
          if (!aWindow.innerWidth || !aWindow.innerHeight)

Error: TypeError: can't access dead object
Source File: chrome://global/content/bindings/findbar.xml
Line: 466


And in _setFoundLink:
            this._foundLink.style.outline = this._tmpOutline;

Error: TypeError: can't access dead object
Source File: chrome://global/content/bindings/findbar.xml
Line: 1262

The above eventually fails in "close":
                  fm.setFocus(element, fm.FLAG_NOSCROLL);

Error: NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIFocusManager.setFocus]
Source File: chrome://global/content/bindings/findbar.xml
Line: 1169

Comment 2

[Simon Lindholm]

FWIW, if you're missing STR, here's for the one on line 466:
1. Open a new tab (say, http://xkcd.com/).
2. Bring up the find bar with Ctrl+F, and search for something that occurs on the page (say, 'a').
3. With focus in the find bar, close the tab with Ctrl+W.
4. With focus still remaining in the find bar, search for the same thing in the new tab by pressing enter.

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: patch

This patch modifies findbar to use weak references when storing references to content windows or elements.

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 620578 [details] [diff] [review]
patch

Review of attachment 620578 [details] [diff] [review]:
-----------------------------------------------------------------

May we add a test mimicking the steps in comment 2, or one of the cases in comment 0?

Comment 5

[Matt Brubeck (:mbrubeck)]

Sure.

Comment 6

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

Added a browser-chrome test using the steps from comment 2.  I verified that the test fails without the fix, and passes with it.  Pushed to Try:
https://tbpl.mozilla.org/?tree=Try&rev=ad944c9b5968

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 620876 [details] [diff] [review]
patch v2

Review of attachment 620876 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/browser_bug749738.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict";

nit: new line before use strict, so it's visible.

@@ +28,5 @@
> +
> +function load(aTab, aUrl, aCallback) {
> +  aTab.linkedBrowser.addEventListener("load", function onload(aEvent) {
> +    aEvent.currentTarget.removeEventListener("load", onload, true);
> +    aCallback();

I was looking at
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_bug567306.js

that tests also waits for focus to the content window, I'm not sure if it's needed for random orange reasons, but woulnd't hurt, I suppose.

Comment 8

[Matt Brubeck (:mbrubeck)]

Pushed with review comments addressed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45df6d27b390

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/45df6d27b390