Last Comment Bug 495153 - nsIEditor.removeEditActionListener exception in this case when unhighlighting after changing source of iframe
: nsIEditor.removeEditActionListener exception in this case when unhighlighting...
Status: RESOLVED FIXED
: regression, testcase
Product: Toolkit
Classification: Components
Component: Find Toolbar (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9.3a1
Assigned To: Graeme McCutcheon [:graememcc]
:
Mentors:
Depends on:
Blocks: 451286
  Show dependency treegraph
 
Reported: 2009-05-27 17:11 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2009-09-18 15:21 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (391 bytes, text/html)
2009-05-27 17:11 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Patch v1 (12.78 KB, patch)
2009-08-01 05:52 PDT, Graeme McCutcheon [:graememcc]
asaf: review+
Details | Diff | Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2009-05-27 17:11:42 PDT
Created attachment 380009 [details]
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] 2009-08-01 05:52:30 PDT
Created attachment 392084 [details] [diff] [review]
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-09-08 14:20:32 PDT
Comment on attachment 392084 [details] [diff] [review]
Patch v1

r=mano
Comment 3 Graeme McCutcheon [:graememcc] 2009-09-18 15:21:46 PDT
http://hg.mozilla.org/mozilla-central/rev/51e562b1032e

Note You need to log in before you can comment on or make changes to this bug.