321073 - ASSERTION: Should not be called: 'Error' (nsGridLayout2::GetRowCount should not be called)

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Jesse Ruderman]

Steps to reproduce: Load the testcase in a (trunk) debug build.

Result:

###!!! ASSERTION: Should not be called: 'Error', file /Users/admin/trunk/mozilla/layout/xul/base/src/grid/nsGridLayout2.cpp, line 302

Comment 1

[Jesse Ruderman]

299 NS_IMETHODIMP
300 nsGridLayout2::GetRowCount(PRInt32& aRowCount)
301 {
302   NS_ERROR("Should not be called");
303   return NS_OK;
304 }

Not assigning to an out-param and returning NS_OK seems like an odd thing to do...

Comment 2

[Jesse Ruderman]

Attachment: testcase

Comment 3

[Jesse Ruderman]

Btw, the assertion happens about 8 times when I load the testcase.

Comment 4

[Jesse Ruderman]

Andreas, are you interested in fixing this bug?

Comment 5

[Andreas Lange]

I can have a look at it. Even if I know my way around those files now, debugging takes a bit more understanding of the code than just a read-through deCOM does.

I'll fix the return stuff in bug 363879. Is NS_ERROR() best to have when a function is forced by an interface but should not be used in that class, or should I change to some other macro?

Comment 6

[Andreas Lange]

The assert is now at line 81 in nsGridLayout2.h (http://landfill.mozilla.org/mxr-test/seamonkey/source/layout/xul/base/src/grid/nsGridLayout2.h#81).

The call is from GetGrid() in nsGridRowLayout.cpp (http://landfill.mozilla.org/mxr-test/seamonkey/source/layout/xul/base/src/grid/nsGridRowLayout.cpp#148).
The code iterates the children and QI to nsIGridPart, not expecting a GridLayout.

There are a few more such places in the grid code, but I'm not sure it is possible to trigger those. 

Comment 7

[Andreas Lange]

Attachment: Avoid calling the assertion

Avoid grids and get rid of an unused rv at the same time.

Comment 8

[[e-mail]]

Comment on attachment 249963 [details] [diff] [review]
Avoid calling the assertion

IMHO this call is not unexpected, because we already allow <grid><rows><box> and I don't see why we can't allow <grid><rows><grid>.

Instead I would have thought that nsIGridPart::GetRowCount should return 1 and only nsGridRowGroupLayout needs to override it.

Comment 9

[Andreas Lange]

Attachment: Default rowcount to 1 row

> Instead I would have thought that nsIGridPart::GetRowCount should return 1 and
> only nsGridRowGroupLayout needs to override it.

Better. Lets do that.

Comment 10

[[e-mail]]

Comment on attachment 250654 [details] [diff] [review]
Default rowcount to 1 row

Nice diffstats ;-)

Comment 11

[Jesse Ruderman]

I checked in the patch (2007-01-07 21:58).

Comment 12

[Jesse Ruderman]

Crashtest checked in.