292972 - Leak WindowStateHolder if CopyJSProperties() fails

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

The code looks like:

+  *aState = new WindowStateHolder(cx, stateObj, this);
+  NS_ENSURE_TRUE(*aState, NS_ERROR_OUT_OF_MEMORY);
+  nsresult rv = CopyJSProperties(cx, mJSObject, stateObj);
+  NS_ENSURE_SUCCESS(rv, rv);

We need to delete *aState (and null it out, probably) if rv is a failure there.

Comment 1

[Mikael Parknert]

Attachment: fix

Comment 2

[Brian Ryner (not reading)]

Comment on attachment 182868 [details] [diff] [review]
fix

>Index: dom/src/base/nsGlobalWindow.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/dom/src/base/nsGlobalWindow.cpp,v
>retrieving revision 1.735
>diff -u -p -8 -r1.735 nsGlobalWindow.cpp
>--- dom/src/base/nsGlobalWindow.cpp	4 May 2005 23:42:35 -0000	1.735
>+++ dom/src/base/nsGlobalWindow.cpp	7 May 2005 10:55:01 -0000
>@@ -6010,17 +6010,21 @@ nsGlobalWindow::SaveWindowState(nsISuppo
>   // The window state object will root the JSObject.
>   *aState = new WindowStateHolder(cx, stateObj, this);
>   NS_ENSURE_TRUE(*aState, NS_ERROR_OUT_OF_MEMORY);
> 
> #ifdef DEBUG_PAGE_CACHE
>   printf("saving window state, stateObj = %p\n", stateObj);
> #endif
>   nsresult rv = CopyJSProperties(cx, mJSObject, stateObj);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  if(NS_FAILED(rv)) {

Space after |if| to keep with the style.

Looks ok otherwise, but I'm curious if you've found a situation where
CopyJSProperties fails?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 182868 [details] [diff] [review]
fix

Requesting 1.8b2 approval for this simple failure-case leak fix...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Mikael, let me know if you need this checked in, ok?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Did this ever get checked in?

Comment 6

[Brian Ryner (not reading)]

checked in.