The default bug view has changed. See this FAQ.

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




8 years ago
6 years ago


(Reporter: graememcc, Assigned: graememcc)


({crash, testcase})

crash, testcase

Firefox Tracking Flags

(Not tracked)



(2 attachments, 1 obsolete attachment)



8 years ago
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

Comment 1

8 years ago
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.
Assignee: nobody → graememcc_firefox
Attachment #387045 - Flags: superreview?(peterv)
Attachment #387045 - Flags: review?(peterv)

Comment 2

8 years ago
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.
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 = :/
Attachment #387045 - Flags: superreview?(peterv)
Attachment #387045 - Flags: review?(peterv)
Attachment #387045 - Flags: review-
(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

8 years ago
Created attachment 387424 [details] [diff] [review]
Patch v2
Attachment #387045 - Attachment is obsolete: true
Attachment #387424 - Flags: review?(timeless)


8 years ago
Attachment #387424 - Flags: review?(timeless) → review+
The test case for this bug had a bogus </head> tag in place of </title>. (Fix in bug 545405.)
This was checked in July 8, 2009. Should be fixed, right?
Last Resolved: 6 years ago
Keywords: testcase
Resolution: --- → FIXED
Severity: normal → critical
Keywords: crash
You need to log in before you can comment on or make changes to this bug.