The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

()

Core
Editor
--
critical
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: graememcc, Assigned: graememcc)

Tracking

({crash, testcase})

Trunk
x86
Linux
crash, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

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
                Aborted"
(Assignee)

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
Status: NEW → ASSIGNED
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.

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 = :/
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.
(Assignee)

Comment 4

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

Updated

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?
http://hg.mozilla.org/mozilla-central/rev/90ec94bdbe1f
Status: ASSIGNED → RESOLVED
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.