Closed Bug 422546 Opened 16 years ago Closed 16 years ago

Topcrash [@ nsEditor::nsEditor] -- trying to addref a garbage gTypingTxnName?

Categories

(Core :: DOM: Editor, defect)

x86
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla1.9beta5

People

(Reporter: jruderman, Assigned: dbaron)

Details

(Keywords: crash, topcrash)

Crash Data

Attachments

(1 file)

According to
http://crash-stats.mozilla.com/topcrasher/byversion/Firefox/3.0b4
[@ nsEditor::nsEditor] is topcrash #33 overall and topcrash #10 on mac.
Example: bp-d114602b-f082-11dc-9746-001a4bd43ef6

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/editor/libeditor/base/nsEditor.cpp&rev=1.508&mark=167,180-186#167

The assumption in the highlighted comment does not hold, as you can see by adding

#ifdef DEBUG
    nsITransactionManager *tm = mTxnMgr;
    PRUint32 v;
    NS_ADDREF(tm);
    NS_RELEASE2(tm, v);
    NS_ASSERTION(v == 1, "Expect to deref a garbage gTypingTxnName later!");
#endif

and then simply opening and closing Firefox.

This "optimization" of holding onto an atom for the string "Typing" seems kinda sketchy to me, but I'm not very familiar with atoms or editor code.
Flags: blocking1.9?
Yeah, I was just looking at the topcrash stats and noticed this one -- the fix is obvious.

This code is broken and will crash if anybody else atomizes the same string and refcounts it such that the other user drops its last reference after the number of editors goes to zero, and then we create another editor.
Assignee: nobody → dbaron
This is around the #10 topcrash on Mac for Firefox 3.0b4 -- and the second highest that's not already fixed and not extension/plugin-related.  (Mac is somewhat less cluttered with extension-related and plugin-related stuff.)
Attached patch patchSplinter Review
I just used nsGkAtoms since editor is part of gklayout now.  (Editor has an atom list already in nsEditProperty, but it's only compiled when HTML-editing is enabled, so using that would break --enable-plaintext-editor-only.)
Attachment #309040 - Flags: superreview?(roc)
Attachment #309040 - Flags: review?
Attachment #309040 - Flags: review? → review+
Comment on attachment 309040 [details] [diff] [review]
patch

ouch
Attachment #309040 - Flags: superreview?(roc) → superreview+
Comment on attachment 309040 [details] [diff] [review]
patch

Low risk fix to a topcrash affecting Mac: probably after closing the last window and then opening more windows again, if documents containing certain tag or attribute names -- "Typing", "IME", or "Deleting" (or something else atomized) were loaded in the last window closed.
Attachment #309040 - Flags: approval1.9?
Comment on attachment 309040 [details] [diff] [review]
patch

a1.9+=damons
Attachment #309040 - Flags: approval1.9? → approval1.9+
Fix checked in to trunk, 2008-03-13 11:54 -0700.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta5
This patch fixed the topcrash.  No crashes were reported in 2008-03-14 through 2008-03-17 nightly builds, down from 3-4 per Mac build.
Status: RESOLVED → VERIFIED
Crash Signature: [@ nsEditor::nsEditor]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: