Closed Bug 522141 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: taras.mozilla, Assigned: taras.mozilla)

References

Details

(Keywords: topcrash, Whiteboard: [#3 trunk (3.7a1) topcrash caused by first patch and fixed by third])

Crash Data

Attachments

(2 files, 1 obsolete file)

Ben Turner debugged this one on windows where failing to close a mmapped file prevents a new one from being created.
Attachment #406117 - Flags: review?(brendan)
Attachment #406117 - Flags: review?(brendan) → review+
Keywords: checkin-needed
Assignee: nobody → tglek
http://hg.mozilla.org/mozilla-central/rev/a7a756bc341b
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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.
(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
Attached patch safety fix (obsolete) — Splinter Review
i doubt that this is a likely crash, but doesn't hurt to guard against a null deref.
Attachment #406764 - Flags: review?(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,
Attached patch safety fixSplinter Review
Attachment #406764 - Attachment is obsolete: true
Attachment #406764 - Flags: review?(brendan)
Attachment #406771 - Flags: review?(brendan)
Comment on attachment 406771 [details] [diff] [review]
safety fix

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

/be
Attachment #406771 - Flags: review?(brendan) → review+
Keywords: topcrash
Summary: FastLoadFileReader fails to close if an error occurs during opening → FastLoadFileReader fails to close if an error occurs during opening [@nsCOMPtr_base::assign_with_AddRef(nsISupports*) ]
Whiteboard: [#3 trunk (3.7a1) topcrash caused by first patch and fixed by third]
Group: core-security
Flags: blocking1.9.2?
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.
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.
No longer blocks: PoisonFrameCrash
Group: core-security
Flags: blocking1.9.2?
Crash Signature: [@nsCOMPtr_base::assign_with_AddRef(nsISupports*) ]
You need to log in before you can comment on or make changes to this bug.