237844 - nsSecureBrowserUIImpl::~nsSecureBrowserUIImpl doesn't need to removeObserver

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[timeless]

mozilla pre1.7a opt profile build wxp
i'm running mozilla under purify, and it has a tendency to crash following this:

        [E] IPR: Invalid pointer read in nsCOMPtr_base::~nsCOMPtr_base(void) {1 
occurrence}
            Reading 4 bytes from 0x0d509c6c (4 bytes at 0x0d509c6c illegal)
            Address 0x0d509c6c points into a HeapAlloc'd block in unallocated 
region of heap 0x00ed0000
            Thread ID: 0x8f4
            Error location
            nsCOMPtr_base::~nsCOMPtr_base(void)+0x1c 
[r:\cenzic\mozilla\xpcom\glue\nscomptr.cpp:64 ip=0x01d16910]
            nsSecureBrowserUIImpl::~nsSecureBrowserUIImpl(void)+0x245 
[r:\cenzic\mozilla\security\manager\boot\src\nssecurebrowseruiimpl.cpp:158 
ip=0x05852ab7]
            nsSecureBrowserUIImpl::`scalar deleting destructor'(UINT)+0x1a 
[C:\Documents and Settings\josh\root\cenzic\mozilla\rel-i586-pc-
msvc\dist\bin\components\pipboot.dll ip=0x0585462d]
            nsSecureBrowserUIImpl::Release(void)+0x52 
[r:\cenzic\mozilla\security\manager\boot\src\nssecurebrowseruiimpl.cpp:166 
ip=0x05851750]
            XPCJSRuntime::GCCallback(JSContext *,JSGCStatus)+0xf18 
[r:\cenzic\mozilla\js\src\xpconnect\src\xpcjsruntime.cpp:556 ip=0x04f37fe2]
            jsds_GCCallbackProc+0xa8 [r:\cenzic\mozilla\js\jsd\jsd_xpc.cpp:518 
ip=0x05355018]
            js_GC+0x1d90         [r:\cenzic\mozilla\js\src\jsgc.c:1419 
ip=0x041c5a97]
            ???            [ip=0x086d9810]
            js_ForceGC+0x83      [r:\cenzic\mozilla\js\src\jsgc.c:1000 
ip=0x041c5f06]
            JS_GC+0xad           [r:\cenzic\mozilla\js\src\jsapi.c:1684 
ip=0x04176f5f]
[E] IPR: Invalid pointer read in nsCOMPtr_base::~nsCOMPtr_base(void) {1 
occurrence}
        Reading 4 bytes from 0xaeaeaeb6 (4 bytes at 0xaeaeaeb6 illegal)
        Address 0xaeaeaeb6 points into invalid memory 
        Thread ID: 0x8f4
        Error location
            nsCOMPtr_base::~nsCOMPtr_base(void)+0x3b 
[r:\cenzic\mozilla\xpcom\glue\nscomptr.cpp:64 ip=0x01d1692f]
            nsCOMPtr_base::~nsCOMPtr_base(void)+0x31 
[r:\cenzic\mozilla\xpcom\glue\nscomptr.cpp:64 ip=0x01d16925]
            nsSecureBrowserUIImpl::~nsSecureBrowserUIImpl(void)+0x245 
[r:\cenzic\mozilla\security\manager\boot\src\nssecurebrowseruiimpl.cpp:158 
ip=0x05852ab7]
            nsSecureBrowserUIImpl::`scalar deleting destructor'(UINT)+0x1a 
[C:\Documents and Settings\josh\root\cenzic\mozilla\rel-i586-pc-
msvc\dist\bin\components\pipboot.dll ip=0x0585462d]
            nsSecureBrowserUIImpl::Release(void)+0x52 
[r:\cenzic\mozilla\security\manager\boot\src\nssecurebrowseruiimpl.cpp:166 
ip=0x05851750]
            XPCJSRuntime::GCCallback(JSContext *,JSGCStatus)+0xf18 
[r:\cenzic\mozilla\js\src\xpconnect\src\xpcjsruntime.cpp:556 ip=0x04f37fe2]
            jsds_GCCallbackProc+0xa8 [r:\cenzic\mozilla\js\jsd\jsd_xpc.cpp:518 
ip=0x05355018]
            js_GC+0x1d90         [r:\cenzic\mozilla\js\src\jsgc.c:1419 
ip=0x041c5a97]
            ???            [ip=0x086d9810]
            js_ForceGC+0x83      [r:\cenzic\mozilla\js\src\jsgc.c:1000 
ip=0x041c5f06]

The code is:
nsSecureBrowserUIImpl::~nsSecureBrowserUIImpl()
{
  nsresult rv;
  // remove self from form post notifications:
  nsCOMPtr<nsIObserverService> svc(do_GetService("@mozilla.org/observer-
service;1", &rv));
  if (NS_SUCCEEDED(rv)) {
    svc->RemoveObserver(this, NS_FORMSUBMIT_SUBJECT);
  }
  if (mTransferringRequests.ops) {
    PL_DHashTableFinish(&mTransferringRequests);
    mTransferringRequests.ops = nsnull;
  }
}

NS_IMPL_ISUPPORTS6(nsSecureBrowserUIImpl,
                   nsISecureBrowserUI,
                   nsIWebProgressListener,
                   nsIFormSubmitObserver,
                   nsIObserver,
                   nsISupportsWeakReference,
                   nsISSLStatusProvider)

From the excerpt, you can see that nsSecureBrowserUIImpl implements 
weakreference and is trying to ask the observerservice to unregister itself as 
part of its destructor.

Comment 1

[timeless]

Attachment: remove it [Checkin: Comment 6]

Comment 2

[Alec Flett]

Comment on attachment 144247 [details] [diff] [review]
remove it
[Checkin: Comment 6]

sr=alecf

Comment 3

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

Removal of observers is a good thing since it prevents multiple objects from
accumulating in the observer service's observer list (which won't be cleared
until XPCOM shutdown, as far as I can tell, although I vaguely remember code
I may have seen once that removed nulled-out weak references while iterating
an observer list to notify the observers).

Also, I don't see how the code you removed would have caused the problems in
comment 0.

Comment 4

[Kai Engert [:KaiE:]]

If I understand correctly, dbaron vetoed to the patch.
Should we resolve this bug as invalid?

Comment 5

[timeless]

(just a note...) 0xaeaeaeae is purify's ABR marker

Comment 6

[Serge Gautherie (:sgautherie)]

Comment on attachment 144247 [details] [diff] [review]
remove it
[Checkin: Comment 6]

{{
1.35	timeless%mozdev.org	2004-04-12 22:16	 	Bug 237844 nsSecureBrowserUIImpl::~nsSecureBrowserUIImpl doesn't need to removeObserver
r=jgmyers sr=alecf
}}

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to comment #3)
> although I vaguely remember code
> I may have seen once that removed nulled-out weak references while iterating
> an observer list to notify the observers).

Exactly:
<http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpcom/ds/nsObserverList.cpp&rev=3.43&mark=103,109-110#96>

> Also, I don't see how the code you removed would have caused the problems in
> comment 0.

R.Fixed (for Gecko v1.8a1), based on the summary of this bug.

(Yet, I wonder as David did.)
timeless, please file a follow-up bug if you think there is something else to fix...