97516 - crash in nsRenderingContextWin::GetWidth() in certain situation

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Shanjian Li]

This bug was sprawn from bugscape 8832. It is reproducible an embed product. I 
believe this problem also happens in certain situation with other mozilla based 
product. 

In following code, 
      nsFontWin** end = &metrics->mLoadedFonts[metrics->mLoadedFontsCount];
      while (font < end) {
        if (FONT_HAS_GLYPH((*font)->mMap, c)) {
          currFont = *font;
          goto FoundFont; // for speed -- avoid "if" statement
        }
mLoadedFontsCount is not initiated. In certain situation, (font < end) is true, 
but *font is null or invalide value, that will lead to a crash.

Comment 1

[Shanjian Li]

Attachment: proposed patch

Comment 2

[Chase Tingley]

confirming

Comment 3

[Shanjian Li]

accepting

Comment 4

[Shanjian Li]

roy review the patch in bugscape 8832.

Comment 5

[Shanjian Li]

Attachment: update patch as suggested by brendan

Comment 6

[Brendan Eich [:brendan]]

Looks ok to me, but I'd like r=yokoyama on this latest patch only, recorded
here, and then one of the SRs cc'd can sr= this patch, too.

/be

Comment 7

[rbs]

Interesting, the nsFontMetricsWin class has 
NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW, defined in
http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsCRT.h#51

Isn't this supposed to zero everything, including therefore mLoadedFontsCount?

Comment 8

[Brendan Eich [:brendan]]

Let's ask scc!

/be

Comment 9

[Shanjian Li]

I finally reproduce the problem with a testcase found in bugscape 9036. The real 
problem is that sometimes null pointer is assigned to mLoadedFonts array. The 
previous patch (attachment 45784 [details]) does not fix any problem. 

Comment 10

[Shanjian Li]

Attachment: The patch that fix the real problem

Comment 11

[Shanjian Li]

FYI, I still could not reproduce the problem with trunk build, but I believe the 
problem exist. We only do not know how to make it happen. I also checked all 
places where mLoadedFonts array is assigned, and everything seems fine except 
the problematic pointer as shown in my patch. 

Comment 12

[rbs]

Yep, that patch looks more sensible to me. A fall-through error code path
that wasn't accounted for. I had this corrected in my font changes.

r=rbs

Comment 13

[Chris Waterson]

Comment on attachment 48340 [details] [diff] [review]
The patch that fix the real problem

sr=waterson

Comment 14

[Shanjian Li]

fix checked in to trunk. 

Comment 15

[Asa Dotzler [:asa]]

Comment on attachment 48340 [details] [diff] [review]
The patch that fix the real problem

a=asa for checkin to 0.9.4 branch

Comment 16

[Shanjian Li]

fix checked in  to 0.9.4 branch. 

Comment 17

[Chris Petersen]

Marking verified per last comments.