235913 - Random crashes while printing on Solaris

Status: RESOLVED FIXED

(Core Graveyard :: Printing: Xprint, defect, P1)

Description

[Roland Mainz]

After bug 224337 ("Mozilla crashes viewing hectorplasmic.com") was fixed many
users started to complain about random crashes when printing.
A stack trace subitted via email looks like this:
-- snip --
% pstack core
core 'core' of 27322:   ./mozilla-bin
-----------------  lwp# 1 / thread# 1  --------------------
 fc2c49a0 __1cUnsFontXlibSubstituteHConvert6MpkHIpHI_I_ (968288, 6c65, 0,
ffbe64b8, 7ef3d0, 6c65) + 308
 fc2c59b4
__1cRnsFontMetricsXlibPFindNearestSize6MpnRnsFontStretchXlib_H_pnKnsFontXlib__
(968288, 0, 0, fc2ddfce, a, fc2f0f9c) + 24
 fc2ca29c
__1cMGetFontNames6FpnYnsFontMetricsXlibContext_pkciipnTnsFontNodeArrayXlib__v_
(7ee4c0, ffbe6b20, ffbe68ec, 0, fc2ca158, 5) + 1b1c
 fd13837c
__1cLnsTextFrameLMeasureText6MpnOnsIPresContext_rknRnsHTMLReflowState_rnRnsTextTransformer_pnOnsILineBreaker_rn0AJTextStyle_rn0AOTextReflowData__I_
(93ccfc, 0, ffbe6d88, ffbe6af8, ffbe6adc, ffbe6c34) + 8c8
 fd1391a4
__1cLnsTextFrameGReflow6MpnOnsIPresContext_rnTnsHTMLReflowMetrics_rknRnsHTMLReflowState_rI_I_
(93ccfc, 981fa0, ffbe6d3c, 0, ffbe6ef4, 1) + 588
 fd105eec
__1cMnsLineLayoutLReflowFrame6MpnInsIFrame_rIpnTnsHTMLReflowMetrics_ri_I_
(ffbe7008, 93ccfc, ffbe6ef4, 0, ffbe6ef0, fd778638) + 498
 fd0cfec8
__1cMnsBlockFrameRReflowInlineFrame6MrnSnsBlockReflowState_rnMnsLineLayout_nTnsLineList_iterator_pnInsIFrame_pC_I_
(93cc80, ffbe77e0, ffbe7008, ffbe6f8c, 93ccfc, ffbe6f97) + 84
 fd0cfb78
__1cMnsBlockFrameUDoReflowInlineFrames6MrnSnsBlockReflowState_rnMnsLineLayout_nTnsLineList_iterator_pipCii_I_
(93cc80, ffbe77e0, ffbe7008, ffbe7004, 93cf20, 80000000) + 208
 fd0cf8e4
__1cMnsBlockFrameYDoReflowInlineFramesAuto6MrnSnsBlockReflowState_nTnsLineList_iterator_pipCii_I_
(93cc80, ffbe77e0, ffbe74ac, ffbe7704, ffbe74b7, 0) + 78
 fd0cf768
__1cMnsBlockFrameSReflowInlineFrames6MrnSnsBlockReflowState_nTnsLineList_iterator_piii_I_
(93cc80, ffbe77e0, ffbe753c, ffbe7704, 0, 0) + 98
 fd0cde44
__1cMnsBlockFrameKReflowLine6MrnSnsBlockReflowState_nTnsLineList_iterator_pii_I_
(93cc80, ffbe77e0, 80000000, ffbe7704, 0, 0) + 63c
 fd0cd250 __1cMnsBlockFrameQReflowDirtyLines6MrnSnsBlockReflowState__I_ (93cc80,
ffbe77e0, 0, 93cf20, 0, 93ccb8) + 2e8
 fd0cb578
__1cMnsBlockFrameGReflow6MpnOnsIPresContext_rnTnsHTMLReflowMetrics_rknRnsHTMLReflowState_rI_I_
(93cc80, 981fa0, ffbe7d40, ffbe7b68, ffbe7c1c, 800000) + 4f0
 fd0d5900
__1cUnsBlockReflowContextLReflowBlock6MrknGnsRect_irnSnsCollapsingMargin_irnInsMargin_rnRnsHTMLReflowState_rI_I_
(ffbe7cfc, 93cc80, ffbe7d28, 35ebe8, 40000000, ffbe8448) + 5d0
 fd0cf17c
__1cMnsBlockFrameQReflowBlockFrame6MrnSnsBlockReflowState_nTnsLineList_iterator_pi_I_
(972610, ffbe80c0, ffbe8448, ffbe7fe4, 40000000, ffbe8120) + 304
 fd0cd8c0
__1cMnsBlockFrameKReflowLine6MrnSnsBlockReflowState_nTnsLineList_iterator_pii_I_
(972610, ffbe80c0, 0, ffbe7fe4, 0, ffbe7fb8) + b8
 fd0cd250 __1cMnsBlockFrameQReflowDirtyLines6MrnSnsBlockReflowState__I_ (972610,
ffbe80c0, 0, 93d14c, 0, 972648) + 2e8
 fd0cb578
__1cMnsBlockFrameGReflow6MpnOnsIPresContext_rnTnsHTMLReflowMetrics_rknRnsHTMLReflowState_rI_I_
(972610, 981fa0, ffbe8620, ffbe8448, ffbe84fc, 800000) + 4f0
 fd0d5900
__1cUnsBlockReflowContextLReflowBlock6MrknGnsRect_irnSnsCollapsingMargin_irnInsMargin_rnRnsHTMLReflowState_rI_I_
(ffbe85dc, 972610, ffbe8608, 35ebe8, 40000000, ffbe8d28) + 5d0
 fd0cf17c
__1cMnsBlockFrameQReflowBlockFrame6MrnSnsBlockReflowState_nTnsLineList_iterator_pi_I_
(77ebe4, ffbe89a0, ffbe8d28, ffbe88c4, 40000000, ffbe8a00) + 304
-- snip --

Comment 1

[Roland Mainz]

Taking myself...

Comment 2

[Roland Mainz]

Attachment: Patch which fixes the problem via allocating the correct size of a CMap

Comment 3

[Roland Mainz]

Comment on attachment 142503 [details] [diff] [review]
Patch which fixes the problem via allocating the correct size of a CMap

Requesting r=/sr= ...

Comment 4

[Jungshik Shin]

Comment on attachment 142503 [details] [diff] [review]
Patch which fixes the problem via allocating the correct size of a CMap


>-  PRUint32 dbmapSize = sizeof(gDoubleByteSpecialCharsCCMap);
>+  PRUint32 dbmapSize = sizeof(gDoubleByteSpecialCharsCCMapUnion);
>   mDoubleByteSpecialCharsCCMap = (PRUint16*)PR_Malloc(dbmapSize);
>   if (!mDoubleByteSpecialCharsCCMap)
>     return NS_ERROR_OUT_OF_MEMORY;

Would this (the line that comes right after the above) be all right? 

 memcpy(mDoubleByteSpecialCharsCCMap, gDoubleByteSpecialCharsCCMap, dbmapSize);

It may be necessary to copy only sizeof(gDoubleByteSpecialCharsCCMap).

Comment 5

[Roland Mainz]

Jungshik Shin wrote:
> (From update of attachment 142503 [details] [diff] [review])
> >-  PRUint32 dbmapSize = sizeof(gDoubleByteSpecialCharsCCMap);
> >+  PRUint32 dbmapSize = sizeof(gDoubleByteSpecialCharsCCMapUnion);
> >   mDoubleByteSpecialCharsCCMap = (PRUint16*)PR_Malloc(dbmapSize);
> >   if (!mDoubleByteSpecialCharsCCMap)
> >     return NS_ERROR_OUT_OF_MEMORY;
>
> Would this (the line that comes right after the above) be all right? 

Yes... that's right... I verified it via adding a printf(). Right now we
malloc() and copy the size of the pointer and not the size of the union - and
that causes the crash (only a common crash on Solaris due some internal details
of how malloc() is implemented in SYSV).

> memcpy(mDoubleByteSpecialCharsCCMap, gDoubleByteSpecialCharsCCMap, dbmapSize);
>
> It may be necessary to copy only sizeof(gDoubleByteSpecialCharsCCMap).

Nope, see above. We would only copy the size of a pointer and not the whole
CCMap data.

Comment 6

[Jungshik Shin]

(In reply to comment #5)
> Jungshik Shin wrote:

> > >+  PRUint32 dbmapSize = sizeof(gDoubleByteSpecialCharsCCMapUnion);

> > Would this (the line that comes right after the above) be all right? 
> 
> Yes... that's right... I verified it via adding a printf(). Right now we
> malloc() and copy the size of the pointer and not the size of the union - and

  My question was not about what you changed but about what you didn't change.
(see below). There's no doubt that the former is necessary.

> 
> > memcpy(mDoubleByteSpecialCharsCCMap, gDoubleByteSpecialCharsCCMap, dbmapSize);
> >
> > It may be necessary to copy only sizeof(gDoubleByteSpecialCharsCCMap).
> 
> Nope, see above. We would only copy the size of a pointer and not the whole
> CCMap data.

 Ok. Then, what I suggested wouldn't work. What I'm worred about is an
'overshooting' by '2bytes' (on 32bit machines). Would that be all right? 

Comment 7

[Roland Mainz]

Jungshik Shin wrote:
> Ok. Then, what I suggested wouldn't work. What I'm worred about is an
> 'overshooting' by '2bytes' (on 32bit machines). Would that be all right? 

I guess yes. Solaris dbx has Purify-like functionality and it did not complain
when I tested the patch ... :)

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #6)
>  Ok. Then, what I suggested wouldn't work. What I'm worred about is an
> 'overshooting' by '2bytes' (on 32bit machines). Would that be all right? 

where would the overshooting come from?
if you're confused by the same thing that I was ;) this is a union, not a
struct, so the align attr does not increase the size.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 142503 [details] [diff] [review]
Patch which fixes the problem via allocating the correct size of a CMap

(jshin: I was asked to review this patch... I hope you don't mind my doing that
even though you had comments about the patch; if you do, sorry...)

Comment 10

[Jungshik Shin]

(In reply to comment #8)

> where would the overshooting come from?
> if you're confused by the same thing that I was ;) this is a union, not a
> struct, so the align attr does not increase the size.

Oh, you're right. Thanks. However, I've just come up with another idea than
using 'sizeof....'. The macro gDoubleByteSpecialCharsCCMap_SIZE is defined 
in gfx/src/x11shared/dbyte_special_chars.ccmap. Instead of this,

   PRUint32 dbmapSize = sizeof(gDoubleByteSpecialCharsCCMapUnion);

we can use

   PRUint32 dbmapSize = gDoubleByteSpecialCharsCCMap_SIZE * sizeof(PRUnichar); 

Either way should be fine, though.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 142503 [details] [diff] [review]
Patch which fixes the problem via allocating the correct size of a CMap

sr=bzbarsky if you use the _SIZE macro as jshin suggests.

For future reference, please diff with -pu8 or something -- the ridiculous lack
of context on this patch made it much more time-consuming to review it.

Comment 12

[Roland Mainz]

Boris Zbarsky wrote:
> (From update of attachment 142503 [details] [diff] [review])
> sr=bzbarsky if you use the _SIZE macro as jshin suggests.

The usage of the _SIZE macro is AFAIK WRONG.
If I recall it correctly the CCMAP macros access the array via |long| datatypes
so a memory read at the last array position may access two bytes of unallocated
memory.

Comment 13

[Roland Mainz]

Comment on attachment 142503 [details] [diff] [review]
Patch which fixes the problem via allocating the correct size of a CMap

Requesting sr= for original patch per last comment.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 142503 [details] [diff] [review]
Patch which fixes the problem via allocating the correct size of a CMap

In that case add some nice clear comments indicating what's going on here.

Comment 15

[Roland Mainz]

Boris Zbarsky wrote:
> (From update of attachment 142503 [details] [diff] [review])
> In that case add some nice clear comments indicating what's going on here

OK... new patch coming in a few secs...

Comment 16

[Roland Mainz]

Attachment: New patch with added comment

Comment 17

[Pierre Chanial]

Comment on attachment 142642 [details] [diff] [review]
New patch with added comment

fix checked in.

Comment 18

[Roland Mainz]

Pierre Chanial wrote:
> (From update of attachment 142642 [details] [diff] [review])
> fix checked in.

Marking bug as FIXED.

Comment 19

[Jungshik Shin]

Hey, roland, you're wrong in comment #12. Pls, don't recall but look into
'gfx/src/x11shared/dbyte_special_chars.ccmap'. 

Comment 20

[Roland Mainz]

Jungshik Shin wrote:
> Hey, roland, you're wrong in comment #12.

The CCMap stuff uses 16bit datatypes for storage - but on RISC machines we have
to use stronger alignment rules since the "natural alignment" (e.g. 16bit
datatypes are 16bit aligned, 32bit datatypes are 32bit aligned, etc.) is not
"enougth" since the CCMap macros which access the data operate with 32bits. That
issue resulted in the crashes fixed with bug 224337.
Now using the _SIZE macros for allocations may not be enougth. When we access
the last entry of the CCMap array we may read one 16bit word beyond the
allocated range. Currently we do not seem to hit that problem because
|gDoubleByteSpecialCharsCCMap_SIZE| is 144 - that perfectly can be dividided
through two or even four without a rest... but when the size changes to
something like 145 (or 147) we will hit the problem.

Comment 21

[Jungshik Shin]

In all cases, |sizeof(....UNION)| is exactly the same as |...._SIZE * sizeof
(PRUnichar)|, which is why I wrote either is fine. Please, don't guess but read
the code.

BTW, the size of a CCMap in terms of bytes is guaranteed to be 0 modulo the size
of ALU. (i.e. multiples of 4 on 32bit machines, multiples of 8 on 64bit machines).