502673 - Possible to crash with multiple document state listeners on the same object

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Graeme McCutcheon [:graememcc]]

Attachment: 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]]

Attachment: 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]

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 (Not reading bugmail)]]

(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]]

Attachment: Patch v2

Comment 5

[Henri Sivonen (:hsivonen)]

The test case for this bug had a bogus </head> tag in place of </title>. (Fix in bug 545405.)

Comment 6

[Daniel Veditz [:dveditz]]

This was checked in July 8, 2009. Should be fixed, right?
http://hg.mozilla.org/mozilla-central/rev/90ec94bdbe1f