Steps to reproduce: Load the testcase in a (trunk) debug build. Result: Two assertions, one in GetMinRowSize and one in GetMaxRowSize. ###!!! ASSERTION: Row index out of range!: 'aRowIndex >=0 && aRowIndex < GetRowCount(aIsHorizontal)', file /Users/admin/trunk/mozilla/layout/xul/base/src/grid/nsGrid.cpp, line 601 ###!!! ASSERTION: Row index out of range!: 'aRowIndex >=0 && aRowIndex < GetRowCount(aIsHorizontal)', file /Users/admin/trunk/mozilla/layout/xul/base/src/grid/nsGrid.cpp, line 616 These two instances of the assertion are followed by runtime checks, but several other similar-looking assertions in the same file are not, so I'm filing this as security-sensitive.

Andreas, want to take a shot at fixing this and/or determining whether it's a security hole?

Ok, I've read through nsGrid.cpp back and forth now, and unless I miss something this is (a) not a security issue and (b) the assertions are bogus. [Using 'Pref' here mening Min|Max|Pref] When we have a broken grid like in the testcase, the grid code will never call GetPrefRowSize since it knows there are no rows or cols. We get here through the grid's layoutmanager, nsGridLayout2, where the strategy for geting sizes is to both do nsStackLayout::GetPrefSize and the grid way and return the max of those. The call to nsStackLayout's GetPrefSize cascades down to nsGridRowLeafLayout::GetPrefSize (at <column>) which simply asks the grid (and thus triggers the assert). As far as I can tell, this is exactly the way it should be. My point about the assertions being bogus comes from this excerpt from the code: NS_ASSERTION(aRowIndex >=0 && aRowIndex < GetRowCount(aIsHorizontal), "Row index out of range!"); nsSize size(0,0); if (!(aRowIndex >=0 && aRowIndex < GetRowCount(aIsHorizontal))) return size; Each assertion is followed by an if that checks the exact same thing and returns a valid response in the same case as when the assertion fired. Thus, the assertion really indicates an invalid xul tree (from the grid's point of view, like the testcase), not a dangerous situation. The reason the assertions fire in the testcase is that the grid counts one row and zero columns - the GetRowCount above returns 0 in the column case. I belive this to be in line with the xul tree and nothing that the source here has any problems with. All places in nsGrid.cpp walking the cellmap iterate 0..(GetRowCount-1) which make them safe in this case (0..0). Those functions in turn call GetRowAt() or GetCellAt(), which are guarded by more assertions. If those assertions fire thing will go wrong, but not even this testcase triggers that. I'll attach a patch which removes those assertions. It also sets the params to CountRowsColumns() to zero, as it might return without touching those values otherwise. Now this function only has one caller which already sets the variables to zero before calling the function, but this felt as good form. (I've no idea how to continue with this bug, please drive it. And bring on any questions!)

> I'll attach a patch which removes those assertions. It also sets the params to That could be misinterpreted. :-) I meant [the patch removes] the assertions that this bug regards, not the ones discussed in the previous paragraph which are real and very much should be kept there!.

Neil, this patch has been waiting for your super-review for a while.

Comment on attachment 249296 [details] [diff] [review] remove assertions Sorry, I've no idea how I overlooked this.

Patch checked in.

These assertions happen on the 1.8 branch, but according to the patch it looks like this was not a security problem. Can I un-hide the bug?

Crashtest checked in.