919318 - Drop the Get prefix on the frame methods GetFirstContinuation, GetLastContinuation, GetFirstInFlow, GetLastInFlow and also on nsLayoutUtils::GetLastContinuationWithChild, because they never return null.

Status: RESOLVED FIXED

(Core :: Layout, defect)

Comment 1

[Mats Palmgren (inactive)]

Attachment: Drop the Get prefix on the frame methods GetFirstContinuation, GetLastContinuation, GetFirstInFlow, GetLastInFlow and also on nsLayoutUtils::GetLastContinuationWithChild, because they never return nul

This is a simple "sed -i" renaming change, so no need to review that deeply.

Comment 2

[Mats Palmgren (inactive)]

Attachment: A few cosmetic changes.

https://tbpl.mozilla.org/?tree=Try&rev=0ce01adb8d04

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 808318 [details] [diff] [review]
A few cosmetic changes.

>diff --git a/layout/generic/nsSplittableFrame.cpp b/layout/generic/nsSplittableFrame.cpp
[...]
>+nsIFrame*
>+nsSplittableFrame::FirstContinuation() const
> {
>   nsSplittableFrame* firstContinuation = const_cast<nsSplittableFrame*>(this);
>   while (firstContinuation->mPrevContinuation)  {
>     firstContinuation = static_cast<nsSplittableFrame*>(firstContinuation->mPrevContinuation);
>   }
>-  NS_POSTCONDITION(firstContinuation, "illegal state in continuation chain.");
>+  MOZ_ASSERT(firstContinuation, "illegal state in continuation chain.");

While you're tweaking these assertions, it'd probably be good to update the assertion-message, since right now it doesn't really make sense.

It sounds like it's saying "oops, we found a missing link in our our continuation chain!" or something like that -- but that's misleading, because it isn't actually able to detect bugs like that. *Really*, this assertion won't catch anything -- it'll never fail, due to the loop condition above it. (In any situation that would make it fail, we'd actually hit a null-deref crash while evaluating the loop condition first; so it's impossible for it to actually fail.)

So the only purpose of this assertion is as documentation, really. It's like a code-comment, with some (highly-unlikely-to-be-used) teeth. Given that, I think something more documentary like:
 Shouldn't be able to get out of the loop above with a null pointer!
...or...
 FirstContinuation() is violating its promise to never return null! (how?)
....or something along those lines would be more descriptive/correct than "illegal state in continuation chain".

Same applies to LastContinuation, FirstInFlow, LastInFlow. (and in the nsTextFrame impls, too)


> nsTableCellMap::AppendCell(nsTableCellFrame& aCellFrame,
>                            int32_t           aRowIndex,
>                            bool              aRebuildIfNecessary,
>                            nsIntRect&        aDamageArea)
> {
>-  NS_ASSERTION(&aCellFrame == aCellFrame.FirstInFlow(), "invalid call on continuing frame");
>+  MOZ_ASSERT(&aCellFrame == aCellFrame.FirstInFlow(),
>+             "invalid call on continuing frame");

Are you sure this is worthy of upgrading to NS_ASSERTION?  If we somehow have web content that triggers this, is it better to crash people's debug builds than to spam an assertion-failure?

Same for ::RemoveCell().  tl;d: I'm not convinced that it's a good idea to upgrade assertions arbitrarily to MOZ_ASSERT, unless we've *really* vetted the code-paths that could possibly trigger that assert and ensured that it really can't fail.

(I, at least haven't done that for ::AppendCell/::RemoveCell, so I don't feel comfortable r+'ing that part of the patch. I'm willing to be convinced that's it's sane, if you've done the vetting.)
eCellFrame *)aCellFrame->FirstInFlow(),

> /** Get the cell map for this table frame.  It is not always mCellMap.
>-  * Only the firstInFlow has a legit cell map
>+  * Only the first-in-flow has a legit cell map.
>   */
>+nsTableCellMap*
>+nsTableFrame::GetCellMap() const
>+{
>+  return static_cast<nsTableFrame*>(FirstInFlow())->mCellMap;

I wonder if it'd be worth adding some sort of assertion that continuations must have a null mCellMap? i.e.
 NS_ASSERTION(!GetPrevInFlow() || !mCellMap, "only first-in-flow should have a legit cell map"
or something like that.

I guess most of the above is optional, so r=me regardless, though I only think we should upgrade the AppendCell/RemoveCell assertions if you're really sure that those can't fail (or that them failing would already be catastrophic to the point where we might as well just crash).

Comment 4

[Mats Palmgren (inactive)]

> While you're tweaking these assertions, it'd probably be good to update the
> assertion-message, since right now it doesn't really make sense.

OK, I changed it to MOZ_ASSERT(firstInFlow, "post-condition failed")

> So the only purpose of this assertion is as documentation, really.

Some code analysis tools are helped by (real) assertions.

> Are you sure this is worthy of upgrading to NS_ASSERTION?  If we somehow
> have web content that triggers this, is it better to crash people's debug
> builds than to spam an assertion-failure?

Yes, definitely.  IMHO, NS_ASSERTIONs are less useful -- because *maybe*
someone notices that message and files a bug, and *maybe* a developer
takes an interest in it because he knows it's a "bad" assertion.
(Just in Layout components we have 274 open bugs with "assert" in the
summary, some are well over a decade old.)

I strongly think we should move to MOZ_ASSERT whenever we can because
it's a win-win proposition: either the condition holds, in which case
we end up with code with stronger assertions (win) -or- the condition
does not hold, in which case we have to decide if it's a real bug
and fix it (win) or just a bad assumption in the assertion in which
case in can be removed/tweaked (win).

We should of course be careful which NS_ASSERTIONs we upgrade
because some are just bogus (and thus misleading).

In this case though, I checked that there are no open bugs and I'm
confident the condition holds.  These two methods are only called
from nsTableRowFrame::Append/RemoveFrame resp.
Both assertions were introduced in bug 24057 in rev 3.41:
"3.41 <[e-mail]> 2000-01-15 12:09
fixed printing assertions; more throughly check cell map usage for
contuining frames; fixed bug 24057; r=kmcclusk,cmanske;"
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=/mozilla/layout/tables&command=DIFF_FRAMESET&file=nsCellMap.cpp&rev2=3.41&rev1=3.40

So I think they are testing an important invariant of the cellmap
that really should be fatal (in debug builds).

> >+nsTableFrame::GetCellMap() const
> >+{
> >+  return static_cast<nsTableFrame*>(FirstInFlow())->mCellMap;
> 
> I wonder if it'd be worth adding some sort of assertion that continuations
> must have a null mCellMap?

Yes, but let's do that as a separate bug.
I see that some callers null-check the result so we should probably
remove those checks as well and require that the first-in-flow
has a non-null mCellMap or dying on OOM.

Comment 5

[Mats Palmgren (inactive)]

Attachment: A few cosmetic changes. v2

Fixed/added the MOZ_ASSERTs as requested.

I noticed that the local variable 'lastInFlow' in nsTextFrame::LastContinuation
is a misnomer so I renamed it to 'lastContinuation'.

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #4)
> > So the only purpose of this assertion is as documentation, really.
> 
> Some code analysis tools are helped by (real) assertions.

Sure - wasn't meaning to question the value of the assertion. I was just getting at who the *actual* reader of the assertion-message would be (in this case, almost certainly just people reading the code, rather than people inspecting their debug-build output), as a basis for what would make a better assertion-message.

Anyway; "post-condition failed" is more accurate than the current message, so that sounds good.

> > Are you sure this is worthy of upgrading to NS_ASSERTION?  If we somehow
> > have web content that triggers this, is it better to crash people's debug
> > builds than to spam an assertion-failure?
> 
> I strongly think we should move to MOZ_ASSERT whenever we can
[...]
> We should of course be careful which NS_ASSERTIONs we upgrade
> because some are just bogus (and thus misleading).

(Right, that was my concern; wanted to make sure this wasn't one of those cases)

> In this case though, I checked that there are no open bugs and I'm
> confident the condition holds.

Great -- that sounds good.

Comment 7

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1080eec8fed1
https://hg.mozilla.org/integration/mozilla-inbound/rev/bca6e2908ddb

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1080eec8fed1
https://hg.mozilla.org/mozilla-central/rev/bca6e2908ddb