522141 - FastLoadFileReader fails to close if an error occurs during opening [@nsCOMPtr_base::assign_with_AddRef(nsISupports*) ]

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[(dormant account)]

Attachment: close in destructor

Ben Turner debugged this one on windows where failing to close a mmapped file prevents a new one from being created.

Comment 1

[(dormant account)]

http://hg.mozilla.org/mozilla-central/rev/a7a756bc341b

Comment 2

[[e-mail]]

There's a problem here in that mNumSharpObjects is not initialised if an error occurs during opening, so we now fail to close harder, i.e. we crash ;-)

I don't see an easy fix for the case where ReadFooterPrefix reads in a non-zero mNumSharpObjects but fails to read the object map.

Comment 3

[(dormant account)]

(In reply to comment #2)
> There's a problem here in that mNumSharpObjects is not initialised if an error
> occurs during opening, so we now fail to close harder, i.e. we crash ;-)
> 
> I don't see an easy fix for the case where ReadFooterPrefix reads in a non-zero
> mNumSharpObjects but fails to read the object map.

Good point, hope this isn't an actual crash you ran into. i'll look addressing that soon

Comment 4

[(dormant account)]

Attachment: safety fix

i doubt that this is a likely crash, but doesn't hurt to guard against a null deref.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 406764 [details] [diff] [review]
safety fix

>diff --git a/xpcom/io/nsFastLoadFile.cpp b/xpcom/io/nsFastLoadFile.cpp
>--- a/xpcom/io/nsFastLoadFile.cpp
>+++ b/xpcom/io/nsFastLoadFile.cpp
>@@ -924,7 +924,7 @@ nsFastLoadFileReader::Close()
>         PR_Close(mFd);
>         mFd = nsnull;
>     }
>-    for (PRUint32 i = 0, n = mFooter.mNumSharpObjects; i < n; i++) {
>+    for (PRUint32 i = 0, n = mFooter.mNumSharpObjects; mFooter.mObjectMap && i < n; i++) {

Use an if please -- should Close just return early before the for loop if mObjectMap is null here?

>         nsObjectMapEntry* entry = &mFooter.mObjectMap[i];
>         entry->mReadObject = nsnull;
>     }
>diff --git a/xpcom/proxy/src/nsProxyObjectManager.cpp b/xpcom/proxy/src/nsProxyObjectManager.cpp
>--- a/xpcom/proxy/src/nsProxyObjectManager.cpp
>+++ b/xpcom/proxy/src/nsProxyObjectManager.cpp
>@@ -333,7 +333,7 @@ NS_GetProxyForObject(nsIEventTarget *tar
>         do_GetService(proxyObjMgrCID, &rv);
>     if (NS_FAILED(rv))
>         return rv;
>-    
>+    //getClassObjectByContractID    

What's this? Trailing whitespace and no space after "//" -- looks like a glitch.

/be

>     // and try to get the proxy object
>     //
>     return proxyObjMgr->GetProxyForObject(target, aIID, aObj,

Comment 6

[(dormant account)]

Attachment: safety fix

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 406771 [details] [diff] [review]
safety fix

r=me with a brief comment on what causes that condition.

/be

Comment 8

[(dormant account)]

http://hg.mozilla.org/mozilla-central/rev/1c4fed24fc4c

Comment 11

[(dormant account)]

I dont see a comment 9 in this bug. Goes from 8 to 10(weird). This fix was for trunk changes that haven't landed on 1.9.2.

Comment 13

[Jesse Ruderman]

This is a fixed fastload bug, and should not be confused with an unfixed layout bug just because both of them crash in the same way.