581166 - Silence some spammy NS_ENSURE_TRUE failures in editor code

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Daniel Holbert [:dholbert]]

Filing this bug to fix a few NS_ENSURE_TRUE failures in editor code that have been bugging me. I'll describe each failure & post patches for them.  All of these involve debug builds with fresh profiles.

First issue, STR:
 1. Load http://ftp.mozilla.org/README and http://ftp.mozilla.org/index.html
    (or pretty much any other two websites)
 2. Switch back and forth between the tabs.

ACTUAL RESULTS: On each tab-switch, I get:
> WARNING: NS_ENSURE_TRUE(bCollapsed) failed: file ../../../../mozilla/editor/libeditor/text/nsTextEditRules.cpp, line 1009

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: First issue fix

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Second issue fix

Second issue STR:
 1. Start firefox, going to about:blank (no other pageloads required)
 2. Quit firefox.

ACTUAL RESULTS: This goes by in my console-spew (one of the last things to appear) as Firefox shuts down:
>WARNING: NS_ENSURE_TRUE(aListener) failed: file ../../../../mozilla/editor/libeditor/base/nsEditor.cpp, line 1714
>WARNING: NS_ENSURE_TRUE(aListener) failed: file ../../../../mozilla/editor/libeditor/base/nsEditor.cpp, line 1714

Fix attached.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: First issue fix

(fixed typo)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: First issue fix

(rrgh, attached the old patch-version again -- fixed typo for real this time. :))

Comment 5

[(no longer active)]

Comment on attachment 459576 [details] [diff] [review]
Second issue fix

Hmm, what does the stack to this call look like?  We shouldn't really be trying to remove a null edit action listener.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: backtrace of NS_ENSURE_TRUE failure in RemoveEditActionListener

Here's the stack trace.  This is happening in cycle-collection, after nsPlaintextEditor::cycleCollection::Unlink has run (which sets nsPlaintextEditor.mRules to nsnull).

Then, in nsPlaintextEditor's destructor, we call RemoveEditActionListener on (a QI'd version of) mRules, which is nsnull.  And that spams the NS_ENSURE_TRUE.

Perhaps it'd be better to null-check mRules in the destructor before calling RemoveEditActionListener.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: Second issue fix, v2

This adds a null-check in the destructor, around RemoveEditActionListener, as described in previous comment.  It also re-wraps the comment right before that so it doesn't go over 80 characters.

Comment 8

[(no longer active)]

Comment on attachment 459904 [details] [diff] [review]
Second issue fix, v2

Yes, this is much better.  Thanks!

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: First issue fix, v2 [r=ehsan]

Oops -- my old fix for 'first issue' tested NS_FAILED on a non-nsresult value. (I was in the mindset of replacing NS_ENSURE_SUCCESS instead of replacing NS_ENSURE_TRUE)

Fixed here. Carrying forward r=ehsan.

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 459904 [details] [diff] [review]
Second issue fix, v2

Requesting approval on this bug's patches.

They don't make any real changes to our logic -- they just replace/navigate around two spammy NS_ENSURE_TRUE warnings in debug builds, thereby making it easier to read other (useful) debugging output & identify/debug real issues.

Comment 11

[Bobby Holley (:bholley)]

I'd be very much in favor of this landing soon, since it gets in the way of my personal tab-switching debugging output.

Comment 12

[Daniel Holbert [:dholbert]]

Landed.
part 1:  http://hg.mozilla.org/mozilla-central/rev/7f8e309426e0
part 2:  http://hg.mozilla.org/mozilla-central/rev/e2a021cf691e