919344 - "can't access dead object" after traversing links through quickfind bar

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Simon Lindholm]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20130922030201

Steps to reproduce:

Visit http://forums.xkcd.com/.

Type ' to open the "quick find (links only)" field, type "board index" (the link with that text will receive a focus outline), and press enter to navigate to http://forums.xkcd.com/index.php.

Type ', and again type "board index". This time, the link with the text will not receive focus, and the browser console shows "TypeError: can't access dead object" coming from Finder.jsm:160 and Finder.jsm:98. Press enter to navigate to the same page again (this still works).

Type '. This time, the quickfind bar won't even appear - errors will be thrown from Finder.jsm:160. If you then type Ctrl+F to open the find bar, you will see that it contains |board index'|, i.e., most keyboard input is going to the hidden search input field.

Comment 1

[Tom S. (please needinfo tschuster)]

Attachment: dead-object

Thank you for this really good bug report. So the problem here is that after we navigated the page the _previousLink is dead. We would need a place to reset it, but we don't really now when the page is navigated. (One approximation could be resetting it when all listeners are removed). But I just went with the easiest solution, wrapping it with a try catch.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Seems to me like we need a more robust solution that avoids leaking references to content objects (even if bug 695480 is helping us here).

Comment 3

[Tom S. (please needinfo tschuster)]

I guess we could add an event listener for something like "unload"? I am not sure what the best event would be.

Comment 4

[Tom S. (please needinfo tschuster)]

Comment on attachment 808646 [details] [diff] [review]
dead-object

I am working on a better fix.

Comment 5

[Tom S. (please needinfo tschuster)]

Attachment: dead object v2

I think this is logically a better patch. Every time we remove the selection, we also null the previousLink. The change in tabbrowser.xml should ensure that it's always called on tab change.

The change in keypress is because I realized .document has a similar issue. And well the code never really worked. "commandDispatcher" only exists on XUL documents and not the DOM document that the findbar usually operates on.

Comment 6

[Mike de Boer [:mikedeboer]]

Comment on attachment 808722 [details] [diff] [review]
dead object v2

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

Bonus points for removing the _document reference! Slowly but surely we'll get to a more sane implementation of the findbar with regard to events.

Comment 7

[Dão Gottwald [:dao]]

Why did it become an external responsibility to call removeSelection on location changes, and why is this only done in tabbrowser? How does e.g. the view source window deal with this?

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 808722 [details] [diff] [review]
dead object v2

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

Update: Dão's right. Finder.jsm/ findbar.xml needs to call removeSelection() when the appropriate event is fired. Or do something similar to the `autocomplete` code just below in `onLocationChange`; add a method to findbar.xml that does this.

```js
this.mTabBrowser.getFindBar(this.mTab).onLocationChange(); // Or a more appropriate name, but better than having all that code in tabbrowser.xml
```

If we can hook on to "beforeunload" or "pagehide" events, that would be preferable. I don't think this is really an issue for non-browser windows (ie. view-source).

Comment 9

[Dão Gottwald [:dao]]

(In reply to Mike de Boer [:mikedeboer] from comment #8)
> If we can hook on to "beforeunload" or "pagehide" events, that would be
> preferable.

The find bar module should be able to use a progress listener just like tabbrowser does. Not sure why you'd switch to a pagehide event.

> I don't think this is really an issue for non-browser windows
> (ie. view-source).

Why not?

Comment 10

[Tom S. (please needinfo tschuster)]

Ok, seems like a good idea to have Finder.jsm standalone, especially when other code that is not the findbar wants to use it.

Comment 11

[Tom S. (please needinfo tschuster)]

Attachment: dead-object v3

I am now using onLocationChange to null out the previous link.

Comment 12

[Mike de Boer [:mikedeboer]]

Comment on attachment 809377 [details] [diff] [review]
dead-object v3

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

I like it! One thing though, I don't now a thing about nsIWebProgress. Asking Dão to review this patch, because he prolly DOES know ;)

::: toolkit/modules/Finder.jsm
@@ +420,5 @@
>  
>      return null;
>    },
>  
> +  // Start of nsIWebProgressListener

nit: please change this comment to
```
// Start of nsIWebProgressListener implementation.
```
...matching the comment for `nsIEditActionListener`.

Comment 13

[Tom S. (please needinfo tschuster)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/0bc2f00a75da

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0bc2f00a75da

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

uplift nomination please, with risk assessment.

Comment 16

[Tom S. (please needinfo tschuster)]

Comment on attachment 809377 [details] [diff] [review]
dead-object v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 666816 
User impact if declined: Can make searches impossible
Testing completed (on m-c, etc.): is on Aurora and Beta, there haven't been any bugs
Risk to taking this patch (and alternatives if risky): relatively low, I can't see how it could break something
String or IDL/UUID changes made by this patch:

Comment 17

[Tom S. (please needinfo tschuster)]

Comment on attachment 809377 [details] [diff] [review]
dead-object v3

Like bug 919340, we don't actually have this in 26.