232035 - nsPrintEngine::FindPrintObjectByDOMWin() leaks window ?

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Harshal Pradhan]

nsCOMPtr<nsIDOMWindowInternal> domWin(GetDOMWinForWebShell(aPO->mWebShell));

This leaks since GetDOMWinForWebShell() returns an addrefed pointer. 

(Found during code inspection. I am not even entirely sure when this code is hit.)

Comment 1

[Harshal Pradhan]

Attachment: Make the function return not return a ref

This should fix it. 

I initially thought about making it return already_addrefed etc. But we seem to
be moving more towards not returning refs when not required, so this patch does
that.

Comment 2

[Harshal Pradhan]

dbaron's window counting stuff makes it very easy to prove that this is
indeed a real leak. 

Steps to reproduce (in a windows debug build):

1) Start mozilla. Go to any frameset page.
(say http://www.mozilla.org/quality/browser/standards/html/frame_name.html )

2) Click on the content area of any of the frames to focus it.(select some text)

3) Go to Print preview.

4) Close Print priview. Shutdown mozilla. Note window count at shutdown. 

> --DOMWINDOW == 4
> --DOMWINDOW == 3

I leaked three window instances in the above test.

Comment 3

[Peter Van der Beken [:peterv]]

>  nsIDOMWindowInternal *
>  nsPrintEngine::GetDOMWinForWebShell(nsIWebShell* aWebShell)
>  {
>    NS_ASSERTION(aWebShell, "Pointer is null!");
>  
>    nsCOMPtr<nsIDOMWindow> domWin = do_GetInterface(aWebShell); 
>  
>    nsCOMPtr<nsIDOMWindowInternal> domWinInt(do_QueryInterface(domWin));
> -  if (!domWinInt) return nsnull;
>  
> -  nsIDOMWindowInternal * dw = domWinInt.get();
> -  NS_ADDREF(dw);
> -
> -  return dw;
> +  return domWinInt;
>  }

This is actually equivalent to

NS_ASSERTION(aWebShell, "Pointer is null!");
nsCOMPtr<nsIDOMWindowInternal> domWinInt = do_GetInterface(aWebShell);

Maybe we could just do away with the function?

Comment 4

[Harshal Pradhan]

Attachment: Updated patch

This does what peterv said. Just replaces the function with do_GetInterface()
everywhere. 

This also includes a cleanup from a patch that jst mailed me. It makes this
code use nsIDOMWindow instead of nsIDOMWindowInternal unless necessary, and
also makes another potentially leaky fucntion return alredy_AddRefed<>

Comment 5

[Harshal Pradhan]

Attachment: -w version of the above. It may be (slightly) more easy to review in places

Comment 6

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

Comment on attachment 139845 [details] [diff] [review]
-w version of the above. It may be (slightly) more easy to review in places

Rename FindFocusedDOMWindowInternal to FindFocusedDOMWindow

Fix indentation in the definitions of
nsPrintEngine::IsThereAnIFrameSelected and
nsPrintEngine::SetupToPrintContent and the definition of the latter.

In FindFocusedDOMWindowInternal, it seems a little dodgy to cast
nsIDOMWindowInteral* to nsIDOMWindow* (since it looks like this is used for
equality comparisons).	Ask jst or peterv if they want to let this code
depend on that continuing to work.

With those, r=dbaron.

Comment 7

[Harshal Pradhan]

Attachment: Updated patch

> In FindFocusedDOMWindowInternal, it seems a little dodgy to cast
> nsIDOMWindowInteral* to nsIDOMWindow* (since it looks like this is used for
> equality comparisons).	Ask jst or peterv if they want to let this code

> depend on that continuing to work.

Actually, that part of the patch came from jst, so he should be ok with that.
But I can change it back to a QI if anyone asks me to. Let me know.

I've fixed the rest of the stuff you pointed out.

Comment 8

[Harshal Pradhan]

Attachment: diff -w of above

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 139886 [details] [diff] [review]
diff -w of above

r=jst

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 139886 [details] [diff] [review]
diff -w of above

sr=jst, that is (since dbaron already reviewed).

Comment 11

[Harshal Pradhan]

Checked in.   

Surprisingly (at least it surprised me), this caused a 428 byte increase in mZ
on linux. Windows didnt change much at all. Looks like do_GetInterface() expands
to quite a bit of code?

+428 (+3976/-3548)	text (DATA)
+428 (+3976/-3548)	UNDEF:libgklayout.so:text
+1356	nsPrintEngine::SetupToPrintContent(nsIDeviceContext*, nsIDOMWindow*)
+595	nsPrintEngine::IsWindowsInOurSubTree(nsIDOMWindow*)
+558	nsPrintEngine::EnablePOsForPrinting()
+416	nsPrintEngine::IsThereARangeSelection(nsIDOMWindow*)
+353	nsPrintEngine::FindFocusedDOMWindow()
+353	nsPrintEngine::FindPrintObjectByDOMWin(nsPrintObject*, nsIDOMWindow*)
+274	nsPrintEngine::IsThereAnIFrameSelected(nsIWebShell*, nsIDOMWindow*,
unsigned char&)
+42	nsPrintEngine::Print(nsIPrintSettings*, nsIWebProgressListener*)
+10	nsPrintEngine::PrintPreview(nsIPrintSettings*, nsIDOMWindow*,
nsIWebProgressListener*)
+6	nsPrintEngine::GetIsIFrameSelected(int*)
+6	nsPrintEngine::GetIsRangeSelection(int*)
+5	nsPrintEngine::GetIsFramesetFrameSelected(int*)
+2	.nosyms.text
-2	nsPrintEngine::ShowPrintErrorDialog(unsigned, int)
-112	nsPrintEngine::ReflowPrintObject(nsPrintObject*, int)
-170	nsPrintEngine::IsThereAnIFrameSelected(nsIWebShell*, nsIDOMWindowInternal*,
unsigned char&)
-257	nsPrintEngine::GetDOMWinForWebShell(nsIWebShell*)
-273	nsPrintEngine::FindPrintObjectByDOMWin(nsPrintObject*, nsIDOMWindowInternal*)
-352	nsPrintEngine::FindFocusedDOMWindowInternal()
-416	nsPrintEngine::IsThereARangeSelection(nsIDOMWindowInternal*)
-610	nsPrintEngine::IsWindowsInOurSubTree(nsIDOMWindowInternal*)
-1356	nsPrintEngine::SetupToPrintContent(nsIDeviceContext*, nsIDOMWindowInternal*)