nsScreenWin held past XPCOM shutdown in static nsCOMPtr

VERIFIED FIXED in mozilla0.8

Status

Core Graveyard
GFX
P3
normal
VERIFIED FIXED
18 years ago
9 years ago

People

(Reporter: dbaron, Assigned: Mike Pinkerton (not reading bugmail))

Tracking

(Blocks: 1 bug, {embed, memory-leak, topembed+})

Trunk
mozilla0.8
x86
Windows 98
embed, memory-leak, topembed+
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nsbeta3-])

Attachments

(4 attachments)

(Reporter)

Description

18 years ago
An nsScreenWin is held past XPCOM shutdown in the "static nsCOMPtr<nsIScreen> 
sPrimaryScreen" defined at:
http://lxr.mozilla.org/seamonkey/source/gfx/src/windows/nsDeviceContextWin.cpp#2
39
See bug 43561 for more info.
(Reporter)

Updated

18 years ago
Blocks: 43561
(Reporter)

Updated

18 years ago
Keywords: mlk

Updated

18 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → M21
Don, could you take a look at this.
Assignee: kmcclusk → dcone
Status: ASSIGNED → NEW

Updated

18 years ago
Blocks: 38671

Updated

18 years ago
No longer blocks: 38671
Status: NEW → ASSIGNED
Keywords: mlk → nsbeta3
Target Milestone: M21 → Future
(Reporter)

Updated

18 years ago
Keywords: mlk
Markign nsbeta3-
Whiteboard: [nsbeta3-]
Correction: Marking nsbeta3-
Marking nsbeta3+.
Reassigning to Waqar
Assignee: dcone → waqar
Status: ASSIGNED → NEW
Whiteboard: [nsbeta3-] → [nsbeta3+]
The problem is the primary screen is cached in FindScreen but it is never 
released.

May want to move the code to cache the sPrimaryScreen into the screen manager's 
GetPrimaryScreen, and release the cached primary screen in the Screen Manager's 
dtor.

CC'ing pinkerton since he was the original owner of this code.

Looks like Mac has the same problem:

http://lxr.mozilla.org/seamonkey/source/gfx/src/mac/nsDeviceContextMac.cpp#380
http://lxr.mozilla.org/seamonkey/source/gfx/src/mac/nsDeviceContextMac.cpp#473

May also want to consider avoiding caching the primary screen if its not a 
significant performance hit.

Reassigning to pinkerton@netscape.com

Pink: If your overloaded, I can look it this further.

Assignee: waqar → pinkerton

Comment 7

18 years ago
clearing nsbeta3+ for triage by new owners. Need info: Does anything bad happen 
due to this that will affect users?  
Whiteboard: [nsbeta3+] → [need info]

Comment 8

18 years ago
Kevin, could you help us triage this?  We don't understand why it should be on
our short list of bugs to fix for 9/14.
The nomination came from the embedding team. It was part of their list of top 
footprint/memory bugs.

I think the only real issue with this bug is that we are probably leaking a few 
device handles on WIN95/98 during application shutdown which may not be returned 
to the system.

I suggest marking nsbeta3- at this point. 

Comment 10

18 years ago
Setting to [nsbeta3-]
Whiteboard: [need info] → [nsbeta3-]

Comment 11

18 years ago
can't we make this a static raw ptr and let some service w/ scope release it 
when it goes away?

Updated

18 years ago
Whiteboard: [nsbeta3-]

Comment 12

18 years ago
shouldn't affect near-term embedding clients, all of which use Linux.
nsbeta3-/future
Whiteboard: [nsbeta3-]
(Assignee)

Updated

18 years ago
Keywords: embed
Target Milestone: Future → mozilla0.9
(Assignee)

Updated

17 years ago
Target Milestone: mozilla0.9 → mozilla0.8
(Reporter)

Comment 13

17 years ago
Created attachment 21631 [details] [diff] [review]
partial and *completely untested, not even compiled* patch

Comment 14

17 years ago
it does not compile :-((

C:\moz_sour\mozilla\mozilla\gfx\src\windows>nmake -f makefile.win

Microsoft (R) Program Maintenance-Dienstprogramm: Version 6.00.8168.0
Copyright (C) Microsoft Corp 1988-1998. Alle Rechte vorbehalten.

+++ make: exporting headers
nsDeviceContextWin.cpp
C:\moz_sour\mozilla\mozilla\gfx\src\windows\nsDeviceContextWin.cpp(100) : error
C2724: 'Shutdown' : 'static' sollte nicht fuer Member-Funktionen verwendet werde
n, die ausserhalb der Klasse definiert werden
NMAKE : fatal error U1077: 'C:\PROGRA~1\MICROS~2\VC98\BIN\cl.exe' : Rueckgabe-Co
de '0x2'
Stop.
NMAKE : fatal error U1077: '"C:\PROGRAMME\MICROSOFT VISUAL STUDIO\VC98\BIN\NMAKE
.exe"' : Rueckgabe-Code '0x2'
Stop.


translation: static should not be used for a member functions, which are declared
outside of the class
(Reporter)

Comment 15

17 years ago
Created attachment 21661 [details] [diff] [review]
corrected patch

Comment 16

17 years ago
Created attachment 21727 [details]
bloat log after applying the patch

Comment 17

17 years ago
The pacth compiles, it fixes the assertion under Win98!

Thank you David.
(Reporter)

Updated

17 years ago
Blocks: 60697

Comment 18

17 years ago
couple questions:
1. What assertion?
2. Can you post a before and after bloat/leak log (sorry to nit)?
3. You mention that "we leak" in your comment at the bottom of the patch. Can
you ellaborate on that? Or better yet, whack this to be an nsIModule?
(Reporter)

Comment 19

17 years ago
Either Win98 runs static dtors on a different thread or NS_GetCurrentThread
returns the wrong thread while running static dtors, so threadsafety assertions
from the NS_IMPL_ADDREF/NS_IMPL_RELEASE are triggered on shutdown while
releasing static nsCOMPtrs.  It's really annoying when developing on Win98,
especially for things like the layout regression tests.

I'm not sure whether we should prefer leaks or static nsCOMPtrs.  Of course, I'd
rather see neither.  This patch turns the static nsCOMPtr into a leak because I
wanted to produce a quick patch that would shut up the assertions for those who
want that.

Comment 20

17 years ago
Created attachment 21743 [details]
bloat log without patch

Comment 21

17 years ago
It does not introduce a new leak, nsScreenWin is also leaked without the patch
at least under Win98. I use the patch, it works and do not need to press away
all the assertions anymore.
(Assignee)

Comment 22

17 years ago
fixed.
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 23

17 years ago
Marking verified per last comments
Status: RESOLVED → VERIFIED

Updated

16 years ago
Keywords: topembed+
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.