bzbarsky wrote: I guess we already have these casts around, but those are a good way to crash, in general, since absolutely nothing guarantees that the object passed in will in fact be an nsTreeColumn. Please file a followup bug to eliminate the casts in favor of actually checkin the object type (eg QI to a private non-scriptable interface, or "QI" to the concrete class the way nsStandardURL does, or something).

Comment on attachment 196900 [details] [diff] [review] patch For future reference, diff -up8 is a lot more readable and easier to review. Just put those in your .cvsrc or something? >Index: nsTreeBodyFrame.cpp >@@ -3314,24 +3314,25 @@ >+ nsTreeColumn* col = GetColumnImpl(aCol); >+ if (col) { I think you want to throw here if col is null, since the code used to assume that it wasn't. >@@ -3351,9 +3352,12 @@ >- return ScrollHorzInternal(col->GetX()); >+ return NS_OK; Why are we changing what this method returns? I don't think we want to do that. The rest looks ok.

Comment on attachment 196900 [details] [diff] [review] patch >+ nsTreeColumn* GetColumnImpl(nsITreeColumn* aUnknownCol) { >+ nsRefPtr<nsTreeColumn> colPtr; >+ nsresult rv = aUnknownCol->QueryInterface(kTreeColumnImplCID, >+ getter_AddRefs(colPtr)); >+ nsTreeColumn* col = colPtr; >+ return col; >+ } Just an observation: return colPtr; works (you could then rename it to col).

Comment on attachment 250365 [details] [diff] [review] patch (checked in) The only changes are the failure codes, right? In that case, transferring bz's r+ for you.

That, plus bz comments are fixed, plus colPtr thing you noticed is fixed, plus a null-check is added to GetColumnImpl.

Checking in nsTreeBodyFrame.cpp; /cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp,v <-- nsTreeBodyFrame.cpp new revision: 1.295; previous revision: 1.294 done Checking in nsTreeBodyFrame.h; /cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.h,v <-- nsTreeBodyFrame.h new revision: 1.107; previous revision: 1.106 done Checking in nsTreeColumns.cpp; /cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeColumns.cpp,v <-- nsTreeColumns.cpp new revision: 1.18; previous revision: 1.17 done Checking in nsTreeColumns.h; /cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeColumns.h,v <-- nsTreeColumns.h new revision: 1.6; previous revision: 1.5 done

this is totally broken. you're returning an ns_already_dead_reference pointer. and on some boxes the compilers simply insist on not building. which means the trees are red.

As I said on IRC, I don't see how it is "totally broken". Wrong from the COM point of view, yes. Working in this case, though. The bustage can be fixed by adding .get() to the return in GetColumnImpl (stupid mistake, I even have a sticky note to check for it before submitting a patch :( ). I'll post a patch that makes GetColumnImpl return an already_AddRefed to make COM gods happy.

Comment on attachment 250536 [details] [diff] [review] addendum >+ return dont_AddRef(NS_SUCCEEDED(rv) ? col : nsnull); Don't need the dont_AddRef here, already_AddRefed has a T* constructor.

the NS_SUCCEEDED check is silly. QI promises to null out the value. just use |col| no ?:.

Thanks for the comments. (Gavin fixed the bustage as suggested in comment 10 already)

Checked in, though it needed merging to the bustage fix...