Bugzilla

Assignee

Comment 2

•

19 years ago

This is combination of the code in MathML
http://lxr.mozilla.org/seamonkey/source/layout/mathml/base/src/nsMathMLTokenFrame.cpp#207

 FinishAndStoreOverflow(&aDesiredSize);
  // Act as if there is overflow no matter what. This is a 
  // safety measure to cater for math fonts with metrics that sometimes
  // cause glyphs in the text frames to protrude outside. Without this,
  // such glyphs may be clipped at the painting stage
  // This flag has already been set on the children as well in 
  // SetInitialChildList()
  mState |= NS_FRAME_OUTSIDE_CHILDREN; 

and after this we might end without a overflow property but a set flag

and
http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsBox.cpp#849

    if (GetStateBits() & NS_FRAME_OUTSIDE_CHILDREN) {
      nsRect* overflow = GetOverflowAreaProperty();
      NS_ASSERTION(overflow, "should have overflow area property");
      rect = *overflow;
    }

the only question is which place is more evil.

Assignee

Comment 3

•

19 years ago

I think the outliner code from bug 151375 is not robust enough.

Assignee

Comment 4

•

19 years ago

Attached patch patch — Details — Splinter Review

Assignee: rbs → bernd_mozilla

Status: NEW → ASSIGNED

Attachment #201982 - Flags: superreview?(roc)

Attachment #201982 - Flags: review?(roc)

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

Comment 5

•

19 years ago

The MathML's FinishAndStoreOverflow() was added in bug 151375 by aaronleventhal (attachment 153195 [details] [diff] [review]). I don't think it is doing anything useful there. It is way too early. We are still half-way into the reflow and the desired size isn't computed yet.

Anyway, the effect that MathML wants is described here:

http://lxr.mozilla.org/mozilla/source/layout/generic/nsContainerFrame.cpp#250

    nsRect damageArea;
    PRBool overlap;
    if (NS_FRAME_OUTSIDE_CHILDREN & aFrame->GetStateBits()) {
      // If the child frame has children that leak out of our box
      // then we don't constrain the damageArea to just the childs
      // bounding rect.
      damageArea = aDirtyRect;
      overlap = PR_TRUE;
    }


Constraining the damage area is the reason of bug 96041 and bug 79732 (see also dup bug 15116).

In the case of MathML (which we know for fact that it involves all sort of strange fonts prone to those issues), the outside bit is set to loosen the paint. It was very cheap and simple (and did not involved extra data). But now that an overflowArea property has been added in bug 151375, it will bloat MathML frames a lot if it is created on all MathML frames. So making the GetOverflow() null-safe as you doing in your patch is preferable on the long run. I still need to iterate on aaron's code, though.

Comment 6

•

19 years ago

Setting NS_FRAME_OUTSIDE_CHILDREN without setting the overflow property is incorrect. The property has to be there. I don't mind adding the null-check in nsBox::SyncLayout but we should definitely still have the assertion and fix the code that triggers it.

Comment 7

•

19 years ago

>Setting NS_FRAME_OUTSIDE_CHILDREN without setting the overflow property is
>incorrect. The property has to be there.

Why do you think/want so? And what about the paint? Seems more robust/beneficial to me to relax the existence and null-safe in general, for case where the overflow can be computed on the fly, overloading |nsRect nsIFrame::GetOverflowRect()| -- which is not done yet, I might note. It will be a bloat if the property is created when we have the bounding metrics that already tell us the information needed at no extra cost.

Assignee

Comment 8

•

19 years ago

Attached patch assert — Details — Splinter Review

Robert, you are right we should catch the other "offenders" too.

Assignee

Comment 9

•

19 years ago

I added the assert, I thought it will fire up like hell but no, I could load all testcases at http://lxr.mozilla.org/seamonkey/source/layout/mathml/tests/ and it stayed silent (only some "minor" table paint asserts ...)

Given the code 
nsRect*
nsIFrame::GetOverflowAreaProperty(PRBool aCreateIfNecessary) 
{
  if (!((GetStateBits() & NS_FRAME_OUTSIDE_CHILDREN) || aCreateIfNecessary)) {
    return nsnull;
  }
i think the previous code 
 a) doubled the query
 b) differs from the code elsewhere
 c) did not deal with 0 robust enough.

I believe that Robert is right, what we see here is how fast (roughly 15 months) the consequences of such a hack surface. If a frame does not play by the rules then it should use its own paint routines and not use mechanisms that are used elsewhere.

Comment 10

•

19 years ago

Yep, roc is right. I wonder (not in this bug) if we shouldn't relax that and rely more on |nsRect nsIFrame::GetOverflowRect()|. That would be robust and work in general when the overflow is known without having to be stashed as a property. That will help the MathML situation (i.e., when it is made to work).

Worcester12345

Comment 11

•

19 years ago

Crashes branch:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051107 Firefox/1.5 ID:2005110708

Sébastien Auger

Comment 12

•

19 years ago

(In reply to comment #11)
> Crashes branch:
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051107
> Firefox/1.5 ID:2005110708
> 

we know but it will not make the 1.8 branch, only the trunk for now.
we can expect a patch to be taken for 1.8.1 maybe if it doesn't break anything.

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

Assignee

Comment 13

•

19 years ago

Robert, could you please explain, what I should change to get patch into the tree.

Updated

•

19 years ago

Attachment #201982 - Flags: superreview?(roc)

Attachment #201982 - Flags: superreview+

Attachment #201982 - Flags: review?(roc)

Attachment #201982 - Flags: review+

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

Updated

•

19 years ago

Attachment #202125 - Flags: superreview+

Attachment #202125 - Flags: review+

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

Comment 14

•

19 years ago

Sorry, I forgot to CC so I was not aware of the activity. Please check in both patches.

rbs,
> I wonder (not in this bug) if we shouldn't relax that and
> rely more on |nsRect nsIFrame::GetOverflowRect()|. That would be robust and
> work in general when the overflow is known without having to be stashed as a
> property. That will help the MathML situation (i.e., when it is made to work).

Yeah, we could make GetOverflowRect virtual and avoid stashing a property on MathML frames. But that's a tradeoff of course because it introduces a virtual call into the processing of non-MathML frames.

One question I have: how many MathML frames on average would actually have overflowing glyphs and need the overflow property? Not all, right? If the answer is that most MathML frames wouldn't need it, then I think the correct tradeoff is clearly to use the property and keep GetOverflowRect nonvirtual.

You probably already realize this but the existing hack can still fail in some situations. E.g. if a MathML frame is in a container frame of the same size, then the container will not mark itself as having outside children, and some paints won't reach the MathML frame.

Olli Pettay [:smaug][bugs@pettay.fi]

Comment 15

•

19 years ago

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


Why this?

>+        nsRect bounds = overflowArea ? *overflowArea + box->GetPosition() :
>+                                         bounds = box->GetRect();

Why not 
nsRect bounds = overflowArea ? *overflowArea + box->GetPosition() : box->GetRect();

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

Comment 16

•

19 years ago

Re: Comment #14:

>Yeah, we could make GetOverflowRect virtual and avoid stashing a property on
>MathML frames. But that's a tradeoff of course because it introduces a virtual
>call into the processing of non-MathML frames.

Seems a reasonnable trade-off. We already have tons of virtual functions... You can't be that picky on that one. [BTW, Knuth said in his TeXbook that the reason he used $...$ to delimit the math mode was because it turns out to be too expensive to typeset.]

>One question I have: how many MathML frames on average would actually have
>overflowing glyphs and need the overflow property? Not all, right? If the
>answer is that most MathML frames wouldn't need it, then I think the correct
>tradeoff is clearly to use the property and keep GetOverflowRect nonvirtual.

Italic glyphs (in the math identifier <mi>) are those that tend to protrude outside. Since all this is confined in <math>...</math>, which is often a little rect, it costs very little to paint the extra, if any.

>You probably already realize this but the existing hack can still fail in some
>situations. E.g. if a MathML frame is in a container frame of the same size,
>then the container will not mark itself as having outside children, and some
>paints won't reach the MathML frame.

That's possible indeed, but it won't be likely in the MathML case. There is a special thing that <math>...</math> always does. It pads itself with the italic correction (both left and right) so that it can stand out a bit from the surrounding text, without seeming too ostentatious at the same time (i.e., paints a bit more, if allowed). To see what I mean, try this test.xhtml to see, e.g., the difference between <i> and <mi>.


<?xml version="1.0"?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1 plus MathML 2.0//EN" 
  "http://www.w3.org/TR/MathML2/dtd/xhtml-math11-f.dtd" [
  <!ENTITY mathml "http://www.w3.org/1998/Math/MathML">
]>
<html xmlns="http://www.w3.org/1999/xhtml">  
<head><title>Test</title></head>
<body>

|<i>j</i>|

|<math xmlns="&mathml;"><mi>j</mi></math>|

</body>
</html>

Comment 17

•

19 years ago

(In reply to comment #16)
> Re: Comment #14:
> >Yeah, we could make GetOverflowRect virtual and avoid stashing a property on
> >MathML frames. But that's a tradeoff of course because it introduces a
> >virtualcall into the processing of non-MathML frames.
> 
> Seems a reasonnable trade-off. We already have tons of virtual functions...
> You can't be that picky on that one. [BTW, Knuth said in his TeXbook that the
> reason he used $...$ to delimit the math mode was because it turns out to be
> too expensive to typeset.]

I can so be picky! "We've always sucked" is not a good reason to suck a little bit more.

> >One question I have: how many MathML frames on average would actually have
> >overflowing glyphs and need the overflow property? Not all, right? If the
> >answer is that most MathML frames wouldn't need it, then I think the correct
> >tradeoff is clearly to use the property and keep GetOverflowRect nonvirtual.
> 
> Italic glyphs (in the math identifier <mi>) are those that tend to protrude
> outside. Since all this is confined in <math>...</math>, which is often a
> little rect, it costs very little to paint the extra, if any.

That doesn't really answer my question, though. I guess we'll have to instrument a build and find out.

> >You probably already realize this but the existing hack can still fail in
> >some situations. E.g. if a MathML frame is in a container frame of the same
> >size, then the container will not mark itself as having outside children, and 
> >some paints won't reach the MathML frame.
> 
> That's possible indeed, but it won't be likely in the MathML case. There is a
> special thing that <math>...</math> always does. It pads itself with the
> italic correction (both left and right) so that it can stand out a bit from
> the surrounding text, without seeming too ostentatious at the same time (i.e.,
> paints a bit more, if allowed).

So are you suggesting that all MathML frames set NS_FRAME_OUTSIDE_CHILDREN? I had thought it was just tokens.

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

Comment 18

•

19 years ago

> I had thought it was just tokens.

Yep, that's seem to be enough so far. I took the example of <math> because it is the one likely to be inside a foreign fame beyond control.

But there are other oddities like bug 161155, making me wonder what is going on.

Comment 19

•

19 years ago

It's probably the same thing. If you have an overflowing glyph in a token frame (or any other frame for that matter) with NS_FRAME_OUTSIDE_CHILDREN, but no overflow rect, inside a MathML container frame (not <math>), then the container frame will not get NS_FRAME_OUTSIDE_CHILDREN even though the glyph may spill out of it. Right?

Comment 20

•

19 years ago

That's right, only token frames are setting the bit now. But it still quite suprising that it clips this bad (on that bug)...

Stephen Donner [:stephend] Not actively reading bugmail

Assignee

Comment 21

•

19 years ago

both patches got checked in 2005-11-10 21:30

Status: ASSIGNED → RESOLVED

Closed: 19 years ago

Resolution: --- → FIXED

Sébastien Auger

Updated

•

19 years ago

Flags: blocking1.9a1?

Comment 22

•

19 years ago

Verified FIXED using trunk build 2005-12-04-10 of SeaMonkey on Windows XP with the testcase at: https://bugzilla.mozilla.org/attachment.cgi?id=201936.  No crash.

Status: RESOLVED → VERIFIED

Assignee

Comment 23

•

19 years ago

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

This could go on branch, its pretty secure

Attachment #201982 - Flags: approval1.8.1?

Attachment #201982 - Flags: approval1.8.0.1?

Assignee

Updated

•

19 years ago

Attachment #202125 - Flags: approval1.8.1?

Attachment #202125 - Flags: approval1.8.0.1?

Comment 24

•

19 years ago

Comment on attachment 202125 [details] [diff] [review]
assert 

a=dveditz for 1.8/1.8.0.1 branches. Please add the fixed1.8.1 and fixed1.8.0.1 keywords when checked in.

Attachment #202125 - Flags: approval1.8.1?

Attachment #202125 - Flags: approval1.8.1+

Attachment #202125 - Flags: approval1.8.0.1?

Attachment #202125 - Flags: approval1.8.0.1+

Comment 25

•

19 years ago

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

a=dveditz for 1.8/1.8.0.1 branches. Please add the fixed1.8.1 and fixed1.8.0.1 keywords when checked in.

Attachment #201982 - Flags: approval1.8.1?

Attachment #201982 - Flags: approval1.8.1+

Attachment #201982 - Flags: approval1.8.0.1?

Attachment #201982 - Flags: approval1.8.0.1+

Assignee

Comment 26

•

19 years ago

fixed on branches

Keywords: fixed1.8.0.1, fixed1.8.1

Updated

•

19 years ago

Flags: blocking1.8.1+

Flags: blocking1.8.0.1+

Comment 27

•

19 years ago

bernd, I'm unable to run thunderbird on the branch after this change. 
I get (what seems like but propably isn't) a very large number of assertions hitting this code.

Frame abuses NS_FRAME_OUTSIDE_CHILDREN flag.

It's possible we're trying to bring up the extension compatibility checker dialog instead of the thunderbird mail chrome. I haven't figured out which one yet.

I don't see this on the trunk oddly enough.

Comment 28

•

19 years ago

I am able to eventually get passed all the assertions here. I see it with any window I try bring up: extensions compatibilty, 3-pane, mail compose, extension manager, theme manager etc.

This seems like a pretty bad regression from this bug, it makes debug builds extremely painful to use on the branch.

Assignee

Comment 29

•

19 years ago

I am not religous about the assert, it would however be cool to post a stack trace and then to back the patch out, as long as the ill-doing frame is still on branch
So post a stack and back the thing out if it hurts your work.

Comment 30

•

19 years ago

Well before we back out the assertion. Does the assertion really mean something unexpected or bad has happened? I'm assuming it was added there for a reason. Yet most of our chrome windows (not just Thunderbird's) set this assertion off. Does that mean someone is making wrong assumptions about something?

Jay Patel [:jay]

Comment 31

•

19 years ago

v.fixed on 1.8.0.1 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060109 Firefox/1.5.0.1, no crash using testcase.

I'll leave it up to you guys to deal with the assertions.  Perhaps we need a new bug for that regression?

Keywords: verified1.8.0.1

Bob Clary [:bc] (inactive)

Assignee

Comment 32

•

19 years ago

The assert means, a frame we don't know yet which, but we should know the fellow, sets the NS_FRAME_OUTSIDE_CHILDREN flag without setting the overflow area. 
This is bad because the frame tries to get painted beyond its borders because there are some flaws in its size computation that lead to clipped painting otherwise. This band aid hack is usually nixed if this frame is nested into another frame because it will see no overflow from the bad fellow and don't set the NS_FRAME_OUTSIDE_CHILDREN for itself. So the painting will be clipped at the parent level.

As Scott indicates this is allready fixed on trunk so I really think, one should  indicate which frame is it and back it out.

Tracy Walker [:tracy]

Updated

•

19 years ago

Keywords: fixed1.8.0.1

Comment 33

•

19 years ago

fwiw I see Frame abuses NS_FRAME_OUTSIDE_CHILDREN assert 12 times justing starting Firefox 1.5.0

Comment 34

•

19 years ago

I'm glad I'm not the only one getting bogged down by the assertion!

Bernd since you added this assertion, can you drive it to resolution one way or the other? Just start up firefox or thunderbird and you should see it enough times to get a good stack trace.

Assignee

Comment 35

•

19 years ago

Attached patch backout (obsolete) — Details — Splinter Review

Please let me backout this thing. This are things that need to get the trunk in shape not a branch punishment.  We are sorry for the inconvenience...

Attachment #208353 - Flags: approval1.8.1?

Attachment #208353 - Flags: approval1.8.0.1?

Brian Ryner (not reading)

Comment 36

•

19 years ago

Comment on attachment 208353 [details] [diff] [review]
backout

Not for 1.8.0.1, probably 1.8.0.2

Is this patch reversed? This shows you adding yet another assertion.

Attachment #208353 - Flags: approval1.8.0.2?

Attachment #208353 - Flags: approval1.8.0.1?

Attachment #208353 - Flags: approval1.8.0.1-

Comment 37

•

19 years ago

Attached patch backout assert on the branch — Details — Splinter Review

Bernd, can we just remove the assertion on the branch for now?  This is pretty much making it impossible for me to debug anything on the branch.

Attachment #208353 - Attachment is obsolete: true

Attachment #209257 - Flags: review?(bernd_mozilla)

Attachment #209257 - Flags: approval1.8.1?

Attachment #208353 - Flags: approval1.8.1?

Attachment #208353 - Flags: approval1.8.0.2?

Brian Ryner (not reading)

Assignee

Comment 38

•

19 years ago

Comment on attachment 209257 [details] [diff] [review]
backout assert on the branch

Sure, back this out, I would have done it for a long timne, but I did not get the permission to do so.

Attachment #209257 - Flags: review?(bernd_mozilla) → review+

Comment 39

•

19 years ago

assertion backed out on MOZILLA_1_8_BRANCH

Benjamin Smedberg

Updated

•

19 years ago

Attachment #209257 - Flags: approval1.8.1? → branch-1.8.1+

Ben Turner (not reading bugmail, use the needinfo flag!)

Comment 40

•

18 years ago

Comment on attachment 209257 [details] [diff] [review]
backout assert on the branch

It would be great to get this out of the 1.8.0 branch also.

Attachment #209257 - Flags: approval1.8.0.3?

Attachment #209257 - Flags: approval1.8.0.2?

Comment 41

•

18 years ago

Comment on attachment 209257 [details] [diff] [review]
backout assert on the branch

wait for 1.8.0.3 please

Attachment #209257 - Flags: approval1.8.0.2? → approval1.8.0.2-

Jay Patel [:jay]

Comment 42

•

18 years ago

Comment on attachment 209257 [details] [diff] [review]
backout assert on the branch

Please check in promptly on the 1.8.0 branch.  Thanks!

Attachment #209257 - Flags: approval1.8.0.3? → approval1.8.0.3+