76407 - Trunk crash [@ 0x00000000 - nsCSSFrameConstructor::CantRenderReplacedElement]

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Jay Patel [:jay]]

This is one of the topcrashers in the latest trunk builds under the 0x00000000 
stack signature as reported by Talkback.  Here is the latest stack trace 
followed by a few entries from the Talkback topcrash report:

Incident ID 29194407 
0x00000000 
nsCSSFrameConstructor::CantRenderReplacedElement 
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp, 
line
10162] 
StyleSetImpl::CantRenderReplacedElement 
[d:\builds\seamonkey\mozilla\content\base\src\nsStyleSet.cpp, line 1334] 
FrameManager::HandlePLEvent 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsFrameManager.cpp, line 1008] 
PL_HandleEvent [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line 589] 
PL_ProcessPendingEvents [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, 
line 522] 
_md_EventReceiverProc [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line 
1070] 
nsAppShellService::Run 
[d:\builds\seamonkey\mozilla\xpfe\appshell\src\nsAppShellService.cpp, line 408] 
main1 [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp, line 1012] 
main [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp, line 1303] 
WinMain [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp, line 1321] 
WinMainCRTStartup() 
KERNEL32.DLL + 0x192a6 (0x77e992a6) 

0x00000000 2eeab8c3
         line 
        Build: 2001041612 CrashDate: 2001-04-17 UptimeMinutes: 30  Total: 404 
        OS: Windows NT  5.0 build 2195
         Detailed : http://climate/reports/incidenttemplate.cfm?bbid=29206819
         StackTrace: 
http://climate/reports/stackcommentemail.cfm?dynamicBBID=29206819
     (29206819) URL: www.nvidia.com

0x00000000 a71157de
         line 
        Build: 2001041612 CrashDate: 2001-04-16 UptimeMinutes: 1  Total: 1 
        OS: Windows NT  5.0 build 2195
         Detailed : http://climate/reports/incidenttemplate.cfm?bbid=29194407
         StackTrace: 
http://climate/reports/stackcommentemail.cfm?dynamicBBID=29194407
     (29194407) Comments: Tried to connect to www.wellsfargo.com

0x00000000 eb8b2b27
         line 
        Build: 2001041510 CrashDate: 2001-04-16 UptimeMinutes: 77  Total: 79 
        OS: Windows NT  4.0 build 1381
         Detailed : http://climate/reports/incidenttemplate.cfm?bbid=29163369
         StackTrace: 
http://climate/reports/stackcommentemail.cfm?dynamicBBID=29163369
     (29163369) URL: www.theregister.co.uk.
     (29163369) Comments: Scrolling across the page

0x00000000 f4d27845
         line 
        Build: 2001041510 CrashDate: 2001-04-16 UptimeMinutes: 25  Total: 45 
        OS: Windows NT  5.0 build 2195
         Detailed : http://climate/reports/incidenttemplate.cfm?bbid=29154575
         StackTrace: 
http://climate/reports/stackcommentemail.cfm?dynamicBBID=29154575
     (29154575) URL: news/foo.com (crash on startup)

0x00000000 eb8b2b27
         line 
        Build: 2001041510 CrashDate: 2001-04-15 UptimeMinutes: 2  Total: 2 
        OS: Windows NT  4.0 build 1381
         Detailed : http://climate/reports/incidenttemplate.cfm?bbid=29149770
         StackTrace: 
http://climate/reports/stackcommentemail.cfm?dynamicBBID=29149770
     (29149770) URL: page refreshed-to by www.nationsbank.com
     (29149770) Comments: I had bugzilla.mozilla.org. open and looking at bug 
5298 and had gone to www.nationsbank.com.  It crashed after the page it 
refreshed to was apparently close to or finished loading.

Comment 1

[Jay Patel [:jay]]

Added Trunk and [@ 0x00000000 - 
nsCSSFrameConstructor::CantRenderReplacedElement] to summary for tracking and 
crash, topcrash keywords.

Comment 2

[karnaze (gone)]

This bug is not very useful without a url or test case. Reassigning to waterson.

Comment 3

[Jay Patel [:jay]]

The Talkback entries all have user submitted URLs and comments:

(29206819) URL: www.nvidia.com
(29194407) Comments: Tried to connect to www.wellsfargo.com
(29163369) URL: www.theregister.co.uk.
(29163369) Comments: Scrolling across the page
(29149770) URL: page refreshed-to by www.nationsbank.com
(29149770) Comments: I had bugzilla.mozilla.org. open and looking at bug 
5298 and had gone to www.nationsbank.com.  It crashed after the page it 
refreshed to was apparently close to or finished loading.

Granted, there aren't very helpful steps to reproduce, but that's all we have 
right now.  I'll add more comments as they come in and maybe QA can help 
reproduce.

Comment 4

[Jay Patel [:jay]]

I was able to crash with Win32 build 2001041612 while trying to reproduce bug 
75070 on Win NT and I got a stack similar to the one found under the 0x00000000 
stack signature:

Incident ID 29188399 
 Trigger Time 
                2001-04-16 16:20:04 
 URL
                www.elendor.net
 User Comments 
                just went to the site. before page finished loading boom! 
 Build ID
                2001041612 
 Product ID
                Netscape6.50 
 Platform ID
                Win32 
 Stack Trace

nsCSSFrameConstructor::CantRenderReplacedElement 
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSFrameConstructor.cpp, 
line 10162] 
StyleSetImpl::CantRenderReplacedElement 
[d:\builds\seamonkey\mozilla\content\base\src\nsStyleSet.cpp, line 1334] 
FrameManager::HandlePLEvent 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsFrameManager.cpp, line 1008] 
PL_HandleEvent [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line 589] 
PL_ProcessPendingEvents [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, 
line 522] 
_md_EventReceiverProc [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line 
1070] 

It was easily reproducible with that build, but I no longer crash when going to 
www.elendor.net with today's 2001041804 build.

Comment 5

[Chris Waterson]

Attachment: sprinkle null-checks and assert-botches liberally

Comment 6

[Chris Waterson]

So either one of two things is happening.

1. We're calling CantRenderReplacedElement() on nsIPresShell with a
   null frame pointer, which enqueues an event that will crash as soon
   as it's processed.

   The only two places in the code that call
   nsIPresShell::CantRenderReplacedElement() are nsImageFrame and
   nsObjectFrame, and both dereference ``this'' at least once before
   calling CantRenderReplacedElement().

   So I'd tend to rule this hypothesis out.

2. By the time we get around to processing the event, the frame to which
   the event has been targeted has been destroyed and over-written.

   If this were the case, I'd expect that we'd have a bogus address, not
   0x0000000 so consistently. However, stranger things have happened.

Anyway, the patch above may wallpaper over the problem long enough for us to 
catch it with one of the assert-botches I put in place.

karnaze, attinasi, what do you think?

Comment 7

[karnaze (gone)]

r=karnaze

I have a slight preference for

  if (!aFrame){
    NS_ASSERTION(PR_FALSE, "null frame to CantRenderReplacedElement");
    return NS_ERROR_NULL_POINTER;
  }

Comment 8

[Chris Waterson]

Attachment: meet karnaze 1/2 way :-)

Comment 9

[Chris Waterson]

Ok, I'll meetcha 1/2 way there. Use NS_ERROR(...) instead of NS_ASSERTION(0, 
...)

Comment 10

[Marc Attinasi]

I'd have suspected some other null ptr, but the line number certainly seems to
implicate the frame. The patch looks very reasonable to me. [s]r=attinasi

BTW: 
there are a lot of pointers used in
nsCSSFrameConstructor::CantRenderReplacedElement without being checked. Should
we clean that up or just hope our luck holds out?

Comment 11

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Is the frame null or is it deleted?  If there's 0x00000000 above it on the
stack, wouldn't that mean it's trying to call through a null vtable pointer
rather than through a null pointer?

I know there were some problems a few days ago related to some of Pavlov's
changes where he was sending two |CantRenderReplacedElementEvent|s for the same
frame -- and I think this could cause such a problem since the frame would be
deleted by the handling of the first event (right?).

So might the right wallpaper over this (if it's not already fixed) be to change
FrameManager::CantRenderReplacedElement by changing:

1048 #ifdef NS_DEBUG
1049 // Verify that there isn't already a posted event associated with
1050 // this frame
1051 NS_ASSERTION(!*FindPostedEventFor(aFrame), "frame already has posted event");
1052 #endif

to

if (*FindPostedEventFor(aFrame)) {
  NS_NOTREACHED("frame already has posted event");
  return NS_ERROR_UNEXPECTED;
}

or something like that...?

AFAICT, every call to nsFrameManager::CantRenderReplacedElement has aFrame
initialized with |this| (via the call to PresShell::CantRenderReplacedElement),
so it ought to be a little hard to end up with a null frame (although an
assertion wouldn't be a bad idea, to document that).

Comment 12

[Chris Waterson]

Sure. My glue-gun is still hot. I'll add that as well.

Comment 13

[Asa Dotzler [:asa]]

Are you still shooting for 0.9 on this? If so please email [e-mail]
with a status on you progress. If not please retarget against a later Milestone.
Thanks.

Comment 14

[Fabian Guisset]

Attachment: testcase for the bug (from another bug actually)

Comment 15

[Fabian Guisset]

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

Comment 16

[Matthias Versen [:Matti]]

Bug 76872 is a possible dupe of this bug.
(Please take a look, bug 76872 is crashing 100% ).

Comment 17

[Stuart Parmenter]

waterson: i think it is happening images that don't load are getting told they
failed (file://'s good ol AsyncOpen being non-Async) and we are calling
CantRenderReplace before the frame's Init() method exits.  mmm reentrancy.

Comment 18

[Chris Waterson]

Attachment: real fix, plus nsCOMPtr-ization

Comment 19

[Chris Waterson]

As usual, dbaron is right. (And pavlov, too.) We're ending up with two calls to
CantRenderReplacedElement(), and the second event is taking it in the shorts.
The fix is to simply make sure we don't have an event posted already for the
frame that's about to get replaced.

karnaze/attinasi/dbaron, could you re-review?

Thanks for the test case, Matti!

Comment 20

[Marc Attinasi]

Chris, your change looks good, but shouldn't we maybe have an NS_WARNING when we
find that the event is already posted (where there used to be an assertion)?
That way we can maybe see the problem and check and fix the caller, maybe...
[s]r=attinasi

Comment 21

[Chris Waterson]

I don't think it's a problem that we want to catch. In the test above test case,
some JS is running inline that replaces one missing image src= with another
missing src=. It's certainly kooky, but it should be something we can handle
without complaint, and certainly without crashing. :-)

Comment 22

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

> but it should be something we can handle
> without complaint, and certainly without crashing

I thought that until a week or two ago we handled it without crashing and
without sending two events for the same frame.  But anyway...

r=dbaron

Comment 23

[Chris Waterson]

fix checked in, a=chofmann

Comment 24

[Chris Waterson]

.

Comment 25

[Keyser Sose]

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

Comment 26

[Stuart Parmenter]

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

Comment 27

[Chris Petersen]

Marking verified in the May 21 build.