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

VERIFIED FIXED in mozilla1.9beta5

Status

()

Core
Editor
--
critical
VERIFIED FIXED
10 years ago
7 years ago

People

(Reporter: Jesse Ruderman, Assigned: dbaron)

Tracking

({crash, topcrash})

Trunk
mozilla1.9beta5
x86
Mac OS X
crash, topcrash
Points:
---
Bug Flags:
blocking1.9 ?

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(1 attachment)

(Reporter)

Description

10 years ago
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?
(Assignee)

Comment 1

10 years ago
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
(Assignee)

Comment 2

10 years ago
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.)
(Assignee)

Comment 3

10 years ago
Created attachment 309040 [details] [diff] [review]
patch

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?

Updated

10 years ago
Attachment #309040 - Flags: review? → review+
Comment on attachment 309040 [details] [diff] [review]
patch

ouch
Attachment #309040 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 5

10 years ago
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+
(Assignee)

Comment 7

10 years ago
Fix checked in to trunk, 2008-03-13 11:54 -0700.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta5
(Reporter)

Comment 8

10 years ago
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.