see also bug 105168 buildid: 2002040809 known incidents: 4969198

Changing QA contact

Stack Trace nsLineLayout::VerticalAlignFrames [d:\builds\seamonkey\mozilla\layout\html\base\src\nsLineLayout.cpp, line 2678] nsLineLayout::VerticalAlignLine [d:\builds\seamonkey\mozilla\layout\html\base\src\nsLineLayout.cpp, line 1895] nsBlockFrame::PlaceLine [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockFrame.cpp, line 4002] nsBlockFrame::DoReflowInlineFrames [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockFrame.cpp, line 3648] nsBlockFrame::DoReflowInlineFramesAuto [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockFrame.cpp, line 3498] nsBlockFrame::ReflowInlineFrames [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockFrame.cpp, line 3443] nsBlockFrame::ReflowLine [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockFrame.cpp, line 2605] nsBlockFrame::ReflowDirtyLines [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockFrame.cpp, line 2250] nsBlockFrame::Reflow [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockFrame.cpp, line 846] nsBlockReflowContext::DoReflowBlock [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockReflowContext.cpp, line 581] nsBlockReflowContext::ReflowBlock [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockReflowContext.cpp, line 359] nsBlockFrame::ReflowBlockFrame [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockFrame.cpp, line 3199] nsBlockFrame::ReflowLine [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockFrame.cpp, line 2471] nsBlockFrame::ReflowDirtyLines [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockFrame.cpp, line 2250] nsBlockFrame::Reflow [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockFrame.cpp, line 846] nsBlockReflowContext::DoReflowBlock [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockReflowContext.cpp, line 581] nsBlockReflowContext::ReflowBlock [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockReflowContext.cpp, line 359] nsBlockFrame::ReflowBlockFrame [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockFrame.cpp, line 3199] nsBlockFrame::ReflowLine [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockFrame.cpp, line 2471] nsBlockFrame::ReflowDirtyLines [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockFrame.cpp, line 2250] nsBlockFrame::Reflow [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockFrame.cpp, line 846] nsBlockReflowContext::DoReflowBlock [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockReflowContext.cpp, line 581] nsBlockReflowContext::ReflowBlock [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockReflowContext.cpp, line 359] nsBlockFrame::ReflowBlockFrame [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockFrame.cpp, line 3199] nsBlockFrame::ReflowLine [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockFrame.cpp, line 2471] nsBlockFrame::ReflowDirtyLines [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockFrame.cpp, line 2250] nsBlockFrame::Reflow [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockFrame.cpp, line 846] nsContainerFrame::ReflowChild [d:\builds\seamonkey\mozilla\layout\html\base\src\nsContainerFrame.cpp, line 807] CanvasFrame::Reflow [d:\builds\seamonkey\mozilla\layout\html\base\src\nsHTMLFrame.cpp, line 565] nsBoxToBlockAdaptor::Reflow [d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxToBlockAdaptor.cpp, line 845] nsBoxToBlockAdaptor::DoLayout [d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxToBlockAdaptor.cpp, line 622] nsBox::Layout [d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBox.cpp, line 1052] nsScrollBoxFrame::DoLayout [d:\builds\seamonkey\mozilla\layout\xul\base\src\nsScrollBoxFrame.cpp, line 395] nsBox::Layout [d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBox.cpp, line 1052] nsContainerBox::LayoutChildAt [d:\builds\seamonkey\mozilla\layout\xul\base\src\nsContainerBox.cpp, line 650] nsGfxScrollFrameInner::LayoutBox [d:\builds\seamonkey\mozilla\layout\html\base\src\nsGfxScrollFrame.cpp, line 1063] nsGfxScrollFrameInner::Layout [d:\builds\seamonkey\mozilla\layout\html\base\src\nsGfxScrollFrame.cpp, line 1222] nsGfxScrollFrame::DoLayout [d:\builds\seamonkey\mozilla\layout\html\base\src\nsGfxScrollFrame.cpp, line 1071] nsBox::Layout [d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBox.cpp, line 1052] nsBoxFrame::Reflow [d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 1001] nsGfxScrollFrame::Reflow [d:\builds\seamonkey\mozilla\layout\html\base\src\nsGfxScrollFrame.cpp, line 780] nsContainerFrame::ReflowChild [d:\builds\seamonkey\mozilla\layout\html\base\src\nsContainerFrame.cpp, line 807] ViewportFrame::Reflow [d:\builds\seamonkey\mozilla\layout\html\base\src\nsViewportFrame.cpp, line 588] nsHTMLReflowCommand::Dispatch [d:\builds\seamonkey\mozilla\layout\html\base\src\nsHTMLReflowCommand.cpp, line 217] PresShell::ProcessReflowCommand [d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 6280] PresShell::ProcessReflowCommands [d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 6335] ReflowEvent::HandleEvent [d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 6191] PL_HandleEvent [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line 597] PL_ProcessPendingEvents [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line 530] _md_EventReceiverProc [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line 1078] KERNEL32.DLL + 0x24407 (0xbff94407) 0x00648c16

my guess is fm is null in: 2677 fm->GetMaxAscent(fontAscent);

If an error occurs, the layout will probably be screwed, but it's better than to crash anyway.

nsbeta1+.

Comment on attachment 78940 [details] [diff] [review] v1 Patch to add null check I think !fm is preferred

I'd be more comfortable with a review from the layout module owner or peer to make sure we're not just wallpapering over problems.

Comment on attachment 78940 [details] [diff] [review] v1 Patch to add null check sr=attinasi - just like bug 137112, but in a different method.

Ere, do you want me to check this in? Please email me if you do (attinasi@netscape.com)

Actually, I think it might be better to try to make GetFontMetrics() fail safe. As long as it can return null it is correct to check that, but Neil had a good idea that we should try using stock system font if the other one fails. I'd be willing to do that to see if it works always, so let's just hold this for now.

Yes, that sounds like a good idea. Please include rbs@maths.uq.edu.au as he has made significant modifications to the fontMetrics stuff recently, and has though at least a bit about this issue. I had a simple idea to put a static in the DeviceContext so the first fornMetrics instance cna be stashed as a fallback, but I have not given it too much thought.

PATCH is ready, if we decide not to wait (it is an NSBETA1+ bug, after all).

This patch adds yet another layer in the quest for returning a non-null font. timeless, care to give the patch a whirl since you are the one filing these bugs and talkback reports. Below is the documentation upon which the patch is based on. I have capitalized the key points that explain what/why the patch is doing. <doc from MDSN> During initialization, GDI creates a number of predefined objects that any application can use. These objects are called stock objects. With the exception of regions and bitmaps, every object type has at least one defined stock object. An application calls the GetStockObject function to get a handle to a stock object, and the returned handle is then used as a standard object handle. The only difference is that NO NEW MEMORY IS USED BECAUSE NO NEW OBJECT IS CREATED. Also, because the system owns the stock objects, an application is not responsible for deleting the object after use. Calling the DeleteObject function with a stock object does nothing. [...] Several stock fonts are defined in the system, the most useful being SYSTEM_FONT. This font is the default selected into a DC and is used for drawing the text in menus and title bars. [...] THE SYSTEM FONT IS ALWAYS AVAILABLE. Other fonts are available only if they have been installed. [...] YOU SHOULD USE STOCK FONTS ONLY IF THE MAPPING MODE FOR YOUR APPLICATION'S DEVICE CONTEXT IS MM_TEXT. </doc>

i'd love to, but the machines i play this game on have no build env and no debugger. so it's easiest for someone to build a talkback build for me to try. I have my laptop here now, so i could try building on my home machine and testing on my laptop. i'll see if i'm still awake later in which case I can try building and testing. -- note that I tried this morning's build and it took way too much effort to crash, although i eventually crashed here (fixing the DeleteObject-> DeleteDC leak really made crashing much harder...) -- I suppose I should make a custom app that just leaks to make it easier for me to play in low GDI environments.

Cc:ing shanjian for r= on attachment 81595 [details] [diff] [review]. I had tested the patch before attaching it -- i.e., I disabled everything that the substitute codepath does so as to force the execution of the new code that I added. Then, I stepped in the debugger to follow the code and see what happened. Result: the call to GetStockObject() worked fine and a substitute font based on the system font was created properly and was then used to render the usual question mark. What I didn't test was the crash (and that's why I asked timeless to try out the patch in his configurartion). If I can get r=/sr=, I might land this patch so that timeless can try tommorrow's build. timeless, you mentioned a "DeleteObject-> DeleteDC leak". Was this a bug in Mozilla or in another application that you have?

mozilla, i can't find the bug # atm, and i'm still not done w/ my bugmail -- it's almost time for work...

Comment on attachment 81595 [details] [diff] [review] patch to try to make font metrics to always succeed The code looks fine for me. But I would suggest you to test it with a printer. (Forcefully set font to stock font, and try to print it.) When map mode is not MM_TEXT (in printing), should we use stock font or just return nsnull. r=shanjian.

Taking the bug. I printed and I didn't see any particular weirdness (in my environment). The assertion didn't fire and the question marks were printed. There might still be some tweaking to answer shanjian's questions. So it needs to go out and be experimented so that feedback and data can be collected. [upon looking at recent gfx checkins with bonsai, the "DeleteObject->DeleteDC leak" was bug 135535]

Comment on attachment 81595 [details] [diff] [review] patch to try to make font metrics to always succeed flagging with r=shanjian attinasi, sr=?

ok, ere spun a debug build with this patch, and it's working amazingly well, explorer.exe has crashed due to the resource shortage, but mozilla is still working fine (this message composed with mozilla and no resoruces. Things do end up using the fallback font, and there appear to be some significant speed issues. I'm getting a lag of many seconds between when I type and when I see what I typed. [30 at last count, although it appears to be more a function of the number of characters processed per second -- i'm estimating that to be 3.] I've had 2 interesting assertions and a few normal ones. The interesting ones include: ###!!! ASSERTION: attempt to blit with bad DCs: '0', file nsRenderingContextWin.cpp, line 2656 And something at nsFontMetricsWin.cpp line 2404 about MM_TEXT and GetMApMode(aDC) -- I couldn't copy this message because at the time the ms dos prompt didn't have enough resources to paint, it does handle low resources well in general, you get a nice message saying you're low on memory with suggestions about quiting things. The normal ones include: ###!!! ASSERTION: ACK, BAD TARGET NODE: 'targetNode', file nsMenuPopupFrame.cpp, line 600 ###!!! ASSERTION: ACK, BAD TARGET: 'targetDocumentWidget', file nsMenuPopupFrame.cpp, line 623

Comment on attachment 81595 [details] [diff] [review] patch to try to make font metrics to always succeed This looks great, it will fix a lot of bugs. Can we now have a postcondition asserting that the return is never null from LoadSubstituteFont and Find SubstituteFont (or maybe even DeviceContextImpl::GetMetricsFor)? If so, we can remove the checks we have sprinkled around for when it IS null. Given those considerations, sr=attinasi

Were any of the null-check fixes checked in? If so, they should be removed now, right?

A number of them were already checked-in :-( If drivers (whom dbaron is one...) think it is worth getting rid of them right now, then I will do a sweep in the tree and clean the ones that I remember to have seen, and upon attaching a patch, other folks can point at what they remember.

Checked-in on the trunk. Now waiting for approval for the 1.0 branch.

*** Bug 136250 has been marked as a duplicate of this bug. ***

*** Bug 136245 has been marked as a duplicate of this bug. ***

Summary change: Copying the signature from duped bug 136245.

Duped bug 136245 was a topcrash. Bringing that keyword forward so that Talkback automation sees it. Verifying: there are no talkback incidents in Trunk builds (following the checkin on 5/2) under either of these two signatures in current data. Adding [FIXED_ON_TRUNK] to the whiteboard and resolving FIXED. Good work rbs.

verified per Talkback

Adding M1RC2 to summary since this is a topcrasher for Mozilla 1.0 RC2 (under both the nsLineLayout::VerticalAlignFrames and nsTextBoxFrame::GetTextSize stack signatures). Since we have already verified the fix on the Trunk, it should be safe to checkin the patch onto the Mozilla 1.0 branch. I guess we're just waiting for ADT approval?

Maybe drivers are more and more cautious these days? Or perhaps the bug has fallen off their radars? I e-mailed them earlier in comment #26. I will e-mail again.

Comment on attachment 82078 [details] [diff] [review] combined patch with an added postcondition for clarity, flagging the correct patch that was checked in.

Comment on attachment 82078 [details] [diff] [review] combined patch with an added postcondition My understanding of this patch is that everything except the last 2 files are removing null-checks that were added earlier for fixing this crash. If that understanding is correct, then a=dbaron for branch checkin of *just the last 2 files in the patch*.

You understanding is correct. Apart from the real fix, the other things are cleanup worthy for the trunk but not vital for the branch.

(s/You/Your/). Fixed on the m1.0 branch.

*** Bug 136224 has been marked as a duplicate of this bug. ***

See bug #235375. Possible reoccurance of bug?