crash in _purecall | nsImageLoader::DoRedraw(nsRect const*)

RESOLVED FIXED in mozilla8

Status

()

Core
ImageLib
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Robert Kaiser, Assigned: jwir3)

Tracking

({crash})

Trunk
mozilla8
x86
Windows NT
crash
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
This bug was filed from the Socorro interface and is 
report bp-9503ee31-f847-4fde-8a69-a8d802110725 .
============================================================= 

Stack:

0 	mozcrt19.dll 	_purecall 	obj-firefox/memory/jemalloc/crtsrc/purevirt.c:54
1 	xul.dll 	nsImageLoader::DoRedraw 	layout/base/nsImageLoader.cpp:240
2 	xul.dll 	nsImageLoader::OnStopFrame 	
3 	xul.dll 	mozilla::imagelib::Decoder::PostFrameStop 	modules/libpr0n/src/Decoder.cpp:248
4 	xul.dll 	mozilla::imagelib::Decoder::Finish 	
5 	xul.dll 	mozilla::imagelib::RasterImage::ShutdownDecoder 	modules/libpr0n/src/RasterImage.cpp:2214
6 	xul.dll 	mozilla::imagelib::RasterImage::UnlockImage 	
7 	xul.dll 	imgRequestProxy::UnlockImage 	modules/libpr0n/src/imgRequestProxy.cpp:363
8 	xul.dll 	nsDocument::RemoveImage 	content/base/src/nsDocument.cpp:8216
9 	xul.dll 	nsResetStyleData::Destroy 	layout/style/nsRuleNode.h:110


This started rising significantly on 8.0a1 Nightlies with the 20110723 build IDs and higher. It seems like there was residual crash before in that function, but now it's significant, with over 30 crashes with the build ID from the 23rd so far.
(Reporter)

Comment 1

6 years ago
https://crash-stats.mozilla.com/report/list?signature=_purecall%20%7C%20nsImageLoader%3A%3ADoRedraw%28nsRect%20const%2A%29 lists more of those crashes, btw.
Seems like this would be caused by us having a bad frame?
(Reporter)

Updated

6 years ago
tracking-firefox8: --- → ?
(Reporter)

Comment 3

6 years ago
Any chance to find a fix? This continues to be a trunk topcrash (#7 in yesterday's data).
Not sure why this just started happening, but the basic problem is this:

PresShell::NotifyDestroyingFrame only calls mPresContext->StopImagesFor(aFrame) when mIgnoreFrameDestruction is false.  That gets set to true during teardown, in particular, at the start of nsFrameManager::Destroy.

We destroy all the image loaders (which have pointers to frames) in nsPresContext::SetShell(null).

However, in PresShell::Destroy there's a bunch of stuff between "FrameManager()->Destroy()" and "mPresContext->SetShell(nsnull)", and this crash is in code in between the two.

We should probably move the code to destroy all image loaders from nsPresContext::SetShell to a new function called by PresShell::SetIgnoreFrameDestruction
(Assignee)

Updated

6 years ago
Assignee: nobody → sjohnson
Status: NEW → ASSIGNED
Status: ASSIGNED → NEW
Version: unspecified → Trunk
(Assignee)

Comment 5

6 years ago
Created attachment 552580 [details] [diff] [review]
b673984

Patch which I believe fixes this issue by following guidance in comment 4. I am currently unable to reproduce this issue, so any testing assistance would be greatly appreciated.
(Assignee)

Updated

6 years ago
Attachment #552580 - Flags: review?(dholbert)
Attachment #552580 - Flags: review?(dbaron)
Comment on attachment 552580 [details] [diff] [review]
b673984

I think it's probably worth putting a null-check of mPresContext around the call to mPresContext->DestroyImageLoaders() (unless you can show why it's not needed).  With that, r=dbaron
Attachment #552580 - Flags: review?(dbaron) → review+
Comment on attachment 552580 [details] [diff] [review]
b673984

Looks good to me!  (modulo dbaron's suggestion)
Attachment #552580 - Flags: review?(dholbert) → review+
Also, the commit message should describe what you're changing and why, rather than just being the summary of the bug.

If you upload a revised patch addressing those comments (and with the commit message noting reviews), one of us can push it to mozilla-inbound.
(Assignee)

Comment 9

6 years ago
Created attachment 553084 [details] [diff] [review]
b673984 (v2) [r=dbaron,dholbert]

Added null check as per dbaron's comments. Also modified commit message to be more informative. 

Try tbpl link: http://tbpl.mozilla.org/?tree=Try&rev=92029dec2b1c
Attachment #552580 - Attachment is obsolete: true
Attachment #553084 - Flags: review+
Attachment #553084 - Flags: checkin?
(In reply to David Baron [:dbaron] from comment #6)
> I think it's probably worth putting a null-check of mPresContext around the
> call to mPresContext->DestroyImageLoaders()

(In reply to Scott Johnson (:jwir3) from comment #9)
> Added null check as per dbaron's comments.

Looks like your null-check is actually a NS_ABORT_IF_FALSE  -- I think dbaron meant an actual "if" check to keep from crashing in that case, e.g.:
  if (mPresContext) {
    mPresContext->DestroyImageLoaders();
  }

I'm not sure off the top of my head under what conditions it'd be null, but we do bother to null-check mPresContext* up two stack levels from the method you're touching (in PresShell::Destroy), so we're apparently at a point in the code where it could be null.

* http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#1916
(Assignee)

Comment 11

6 years ago
(In reply to Daniel Holbert [:dholbert] from comment #10)
> Looks like your null-check is actually a NS_ABORT_IF_FALSE  -- I think
> dbaron meant an actual "if" check to keep from crashing in that case, e.g.:

Ah, sorry, my mistake. I've corrected this and posted a new patch. Thanks for pointing this out!
(Assignee)

Comment 12

6 years ago
Created attachment 553089 [details] [diff] [review]
b673984 (v3) [r=dbaron,dholbert]

Incorrect null check fixed.
Attachment #553084 - Attachment is obsolete: true
Attachment #553084 - Flags: checkin?
Attachment #553089 - Flags: review+
Attachment #553089 - Flags: checkin?
Landed on mozilla-inbound:
  http://hg.mozilla.org/integration/mozilla-inbound/rev/f9be98f4a042
Status: NEW → ASSIGNED
Whiteboard: [inbound]
Attachment #553089 - Flags: checkin? → checkin+
http://hg.mozilla.org/mozilla-central/rev/f9be98f4a042
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8

Comment 15

6 years ago
I don't see any of these signatures on 8.0a2 and nothing on 8.0a1 in builds after the 16th and nothing on the trunk. I think we are good. I also think we don't need to track this so removing the nom.

Updated

6 years ago
tracking-firefox8: ? → ---
You need to log in before you can comment on or make changes to this bug.