293717 - crash in nsTreeContentView::GetCellText

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Daniel Brooks [:db48x]]

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.

Comment 1

[Daniel Brooks [:db48x]]

Attachment: patch

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Who's passing null columns? Is that allowed? Should the caller be fixed instead?

Comment 3

[timeless]

roc: the api is public(scriptable), impls must accept null and handle it gracefully.

Comment 4

[Daniel Brooks [:db48x]]

Comment on attachment 183252 [details] [diff] [review]
patch

thanks roc :)

Comment 5

[Asa Dotzler [:asa]]

Comment on attachment 183252 [details] [diff] [review]
patch

moving request out to b3. We're very nearly wrapped up on 1.8b2.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

(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.

Comment 7

[Daniel Brooks [:db48x]]

Yes, it's an assertion in a debug build, but in a normal build it returns an
error (NS_NS_ERROR_INVALID_ARG.)

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

but that means that the caller still needs to be fixed.

Comment 9

[Daniel Brooks [:db48x]]

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 10

[Daniel Brooks [:db48x]]

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.

Comment 11

[Daniel Brooks [:db48x]]

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.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

> 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 :)

Comment 13

[Daniel Brooks [:db48x]]

Well, I did, but it was in code I wrote myself :)

Comment 14

[Mike Shaver (:shaver emeritus)]

Comment on attachment 183252 [details] [diff] [review]
patch

a=shaver

Comment 15

[Daniel Brooks [:db48x]]

checked in

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 299926 has been marked as a duplicate of this bug. ***