43590 - nsScreenWin held past XPCOM shutdown in static nsCOMPtr

Status: VERIFIED FIXED

(Core Graveyard :: GFX, defect, P3)

Description

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

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.

Comment 1

[Kevin McCluskey (gone)]

Don, could you take a look at this.

Comment 2

[Kevin McCluskey (gone)]

Markign nsbeta3-

Comment 3

[Kevin McCluskey (gone)]

Correction: Marking nsbeta3-

Comment 4

[Kevin McCluskey (gone)]

Marking nsbeta3+.
Reassigning to Waqar

Comment 5

[Kevin McCluskey (gone)]

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.

Comment 6

[Kevin McCluskey (gone)]

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 [e-mail]

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

Comment 7

[Peter Trudelle]

clearing nsbeta3+ for triage by new owners. Need info: Does anything bad happen 
due to this that will affect users?  

Comment 8

[Peter Trudelle]

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.

Comment 9

[Kevin McCluskey (gone)]

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

[Peter Trudelle]

Setting to [nsbeta3-]

Comment 11

[Judson Valeski]

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

Comment 12

[Peter Trudelle]

shouldn't affect near-term embedding clients, all of which use Linux.
nsbeta3-/future

Comment 13

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

Attachment: partial and *completely untested, not even compiled* patch

Comment 14

[Bernd]

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

Comment 15

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

Attachment: corrected patch

Comment 16

[Bernd]

Attachment: bloat log after applying the patch

Comment 17

[Bernd]

The pacth compiles, it fixes the assertion under Win98!

Thank you David.

Comment 18

[Judson Valeski]

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?

Comment 19

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

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

[Bernd]

Attachment: bloat log without patch

Comment 21

[Bernd]

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.

Comment 22

[Mike Pinkerton (not reading bugmail)]

fixed.

Comment 23

[Chris Petersen]

Marking verified per last comments