495153 - nsIEditor.removeEditActionListener exception in this case when unhighlighting after changing source of iframe

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, to reproduce:
- Open testcase, press ctrl-f
- Search for the letter 'e' and click on 'Highlight all'
- In the testcase, press the 'change location of iframe' button
- Click on 'Highlight all'
- Click again on 'Highlight all'

After that, I see this error in the error console:
Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIEditor.removeEditActionListener]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://global/content/bindings/findbar.xml :: _unhookListenersAtIndex :: line 499"  data: no]

And I think this can also lead somehow to this error (although I have no precise way to reproduce that one):
Error: uncaught exception: [Exception... "Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIEditor.document]"  nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)"  location: "JS frame :: chrome://global/content/bindings/findbar.xml :: _highlightDoc :: line 981"  data: no]
Usually, when this error starts happening, the whole unhighlighting business seems to go wrong partly.

Comment 1

[Graeme McCutcheon [:graememcc]]

Attachment: Patch v1

Part of the problem here is the code in browser.js that reverts the state of the highlight button on an onLocationChange event, in the testcase caused by the changing of the iframe src.

However, the real problem is that the highlight code attaches listeners multiple times if there are multiple matches in the editor. The testcase is analagous to this, because when the onLocationChange causes the button state to be cleared, the findbar internal state isn't cleared - we're already hooked up as a listener from the first highlighting, so when we highlight again we are adding another set of listeners.

The editors themselves cope with this just fine - they do the right thing in the case of duplicate entries. However, at the final unhighlighting, we are calling removeEditActionListener twice on the same editor. The first call succeeds, and then the second time we have already been removed, causing the exception.

Annoyingly I see that I was far-sighted enough when I wrote the code for the listeners within the findbar - they can already cope with multiple matches in one editor. It's just the highlighting code that gets it wrong.

The patch fixes that and extends (and tidies up) the 451540 test to show that everything works correctly.

Comment 2

[Asaf Romano (gone)]

Comment on attachment 392084 [details] [diff] [review]
Patch v1

r=mano

Comment 3

[Graeme McCutcheon [:graememcc]]

http://hg.mozilla.org/mozilla-central/rev/51e562b1032e