Closed Bug 293717 Opened 19 years ago Closed 19 years ago

crash in nsTreeContentView::GetCellText

Categories

(Core :: XUL, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: db48x, Assigned: db48x)

References

Details

(Keywords: crash)

Attachments

(1 file)

This function doesn't test that it's column argument is valid, so it just crashes. 

In fact, out of the 13 implementations of the nsITreeView interface, the only
one that doesn't crash here is nsNSSASN1Tree, and that's just because their tree
only has one column and they never use the parameter. Perhaps if I get some time
later I'll fix them as well.
Attached patch patchSplinter Review
Attachment #183252 - Flags: review?(jan)
Attachment #183252 - Flags: review?(jan) → review+
Attachment #183252 - Flags: superreview?(roc)
Who's passing null columns? Is that allowed? Should the caller be fixed instead?
roc: the api is public(scriptable), impls must accept null and handle it gracefully.
Attachment #183252 - Flags: superreview?(roc) → superreview+
Comment on attachment 183252 [details] [diff] [review]
patch

thanks roc :)
Attachment #183252 - Flags: approval1.8b2?
Comment on attachment 183252 [details] [diff] [review]
patch

moving request out to b3. We're very nearly wrapped up on 1.8b2.
Attachment #183252 - Flags: approval1.8b2? → approval1.8b2+
(In reply to comment #2)
> Who's passing null columns? Is that allowed? Should the caller be fixed instead?

so, the patch makes this an NS_PRECONDITION, which means that this will now be
an assertion but still a bug.
Yes, it's an assertion in a debug build, but in a normal build it returns an
error (NS_NS_ERROR_INVALID_ARG.)
but that means that the caller still needs to be fixed.
Sure, but a crash would would also indicate a bug in the caller. At least this
way there's an error message (assertion, js exception, etc) instead. However,
there isn't any code in the mozilla source that hits this condition, so there's
nothing else to fix. All 13 (C++) implementations of this interface in our tree
are very sloppy, simply because all of the consumers in our tree are fairly well
behaved.
Comment on attachment 183252 [details] [diff] [review]
patch

Also, I double checked with Asa. As his comment says, he meant to set
approval1.8b3? not approval1.8b2+, so I'll fix that.
Attachment #183252 - Flags: approval1.8b2+ → approval1.8b3?
Oh, and I'm sure most of the JS implementations are sloppy too, but at least
they won't crash just because something was null/undefined.
Status: NEW → ASSIGNED
> However, there isn't any code in the mozilla source that hits this condition

oh. sorry about this misunderstanding. I thought you filed this bug because you
hit a crash. so, nevermind :)
Well, I did, but it was in code I wrote myself :)
Comment on attachment 183252 [details] [diff] [review]
patch

a=shaver
Attachment #183252 - Flags: approval1.8b3? → approval1.8b3+
checked in
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
*** Bug 299926 has been marked as a duplicate of this bug. ***
Component: XP Toolkit/Widgets: Trees → XUL
QA Contact: xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: