Last Comment Bug 502673 - Possible to crash with multiple document state listeners on the same object
: Possible to crash with multiple document state listeners on the same object
Status: RESOLVED FIXED
: crash, testcase
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: x86 Linux
: -- critical (vote)
: ---
Assigned To: Graeme McCutcheon [:graememcc]
:
: Makoto Kato [:m_kato]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-07-06 11:54 PDT by Graeme McCutcheon [:graememcc]
Modified: 2011-01-03 03:31 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (uses enhanced privileges) (1.81 KB, text/html)
2009-07-06 11:54 PDT, Graeme McCutcheon [:graememcc]
no flags Details
Patch v1 (8.30 KB, patch)
2009-07-06 13:53 PDT, Graeme McCutcheon [:graememcc]
timeless: review-
Details | Diff | Splinter Review
Patch v2 (8.31 KB, patch)
2009-07-08 04:16 PDT, Graeme McCutcheon [:graememcc]
timeless: review+
Details | Diff | Splinter Review

Description Graeme McCutcheon [:graememcc] 2009-07-06 11:54:54 PDT
Created attachment 387021 [details]
Testcase (uses enhanced privileges)

See testcase.

Not likely to be a common scenario, I'll be the first to admit, but:

1. Multiple objects register themselves as a document state listener on an editor
2. During a callback, one of them decides it's no longer interested in notifications, and removes itself as a listener during the callback
3. Crash with  "pure virtual method called
                terminate called without an active exception
                Aborted"
Comment 1 Graeme McCutcheon [:graememcc] 2009-07-06 13:53:24 PDT
Created attachment 387045 [details] [diff] [review]
Patch v1

So, nsEditor::NotifyDocumentListeners does 2 bad things:

1) Assumes a listener won't remove itself during the callback (bad, as it's not prohibited by the API, but not the cause of the crash)

2) When looping over the array of listeners, it reads the size of the array once, and assumes it never changes. This is what gets us in to trouble, of course.

The patch tackles both. Just using mDocStateListeners.Count() in the loop condition wouldn't be sufficient, as it would avoid the crash, but potentially cause a listener to be skipped when sending out notifications (if the previous listener in the array removed itself).

(Incidentally, the notifications for nsIEditActionListeners have that problem).

I can't see an easy way to test for the NotifyDocumentCreated notification at the moment.
Comment 2 timeless 2009-07-07 02:50:13 PDT
Comment on attachment 387045 [details] [diff] [review]
Patch v1

1. i don't think peterv is an editor peer, so please don't ask him to review.

http://www.mozilla.org/owners.html
Composer (Editor/Composer) 

you can ask glazou, neil, brade, smfr, or myself for reviews

>-        rv = mDocStateListeners[i]->NotifyDocumentCreated();
>+        rv =listeners[i]->NotifyDocumentCreated();

please don't accidentally lose spaces after = :/
Comment 3 Chris Pearce (:cpearce) 2009-07-07 15:57:52 PDT
(In reply to comment #2)
> (From update of attachment 387045 [details] [diff] [review])
> 1. i don't think peterv is an editor peer, so please don't ask him to review.

peterv did the contentEditable implementation, and has reviewed every editor patch I've done over the past two years. He's been the most active (the only?) reviewer in editor for a while. Asking him for review is appropriate.

> you can ask glazou, neil, brade, smfr, or myself for reviews

When I started working on editor (I've since moved to <video> about a year ago), I was told that the only person who would respond to review requests for editor was peterv. glazou (the module owner) never responded, peterv did...

If peterv doesn't have the appropriate privileges, he should be given them.
Comment 4 Graeme McCutcheon [:graememcc] 2009-07-08 04:16:34 PDT
Created attachment 387424 [details] [diff] [review]
Patch v2
Comment 5 Henri Sivonen (:hsivonen) 2010-02-17 23:47:42 PST
The test case for this bug had a bogus </head> tag in place of </title>. (Fix in bug 545405.)
Comment 6 Daniel Veditz [:dveditz] 2010-12-19 00:36:26 PST
This was checked in July 8, 2009. Should be fixed, right?
http://hg.mozilla.org/mozilla-central/rev/90ec94bdbe1f

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