Closed Bug 1242456 Opened 8 years ago Closed 8 years ago

Create a RAII helper to manage HGLOBALs and use in ShowNativePrintDialog.

Categories

(Core :: Printing: Output, defect)

46 Branch
All
Windows
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: bobowen, Assigned: so61pi.re, Mentored)

References

Details

(Whiteboard: [good first bug])

Attachments

(1 file, 8 obsolete files)

This should simplify the code in this function and CreateGlobalDevModeAndInit, which it calls, should be looked at as well.
The existing freeing of the HGLOBAL should also be checked.

The helper should be similar to these:
https://dxr.mozilla.org/mozilla-central/source/xpcom/base/nsWindowsHelpers.h#142

See bug 1238964 comment 7.
Hi Bob Owen, I want to work on this bug.
(In reply to so61pi from comment #1)
> Hi Bob Owen, I want to work on this bug.

Excellent, let me know if you have any questions.

Have you got a working build?
I'm building, and will work on it right after I have a working build.
Hi Bob Owen, I've created a patch for this bug, please take a look at it.
Attachment #8716990 - Flags: review?(bobowen.code)
Comment on attachment 8716990 [details] [diff] [review]
Create a RAII helper to manage HGLOBALs and use in ShowNativePrintDialog.

I knew I was going to get into trouble when I set myself as mentor on this, I've only recently come across HGLOBALs. :-)

I think my wording might have been a bit misleading in the description.
The checking of the freeing comment was a side thing I mentioned, because I wasn't sure it was correct.

When Jim asked me to file this bug, in bug 1238964 comment 7, I think he was referring to managing the locking and unlocking of the HGLOBAL.

Jim - is that correct?

I wonder if for HGLOBALs it would be good to have something more sophisticated than an nsAutoRef class, which managed the allocation/freeing and lock/unlock?


Depending on what Jim says this may be irrelevant, but:

embedding/components/printingui/win/nsPrintDialogUtil.cpp
>  // NOTE: We only need to free hGlobalDevMode when the dialog is cancelled
>  // When the user prints, it comes back in the printdlg struct and 
>  // is used and cleaned up later
>  hGlobalDevMode = CreateGlobalDevModeAndInit(printerName, aPrintSettings);
	
I'm not entirely sure this comment is true.
When the user prints I think we copy the DEVMODE into the print settings, so I think we might need to free this but don't.
Flags: needinfo?(jmathies)
Attachment #8716990 - Flags: review?(bobowen.code)
> I knew I was going to get into trouble when I set myself as mentor on this,
> I've only recently come across HGLOBALs. :-)

Sorry about that man :) .

> I think my wording might have been a bit misleading in the description.
> The checking of the freeing comment was a side thing I mentioned, because I
> wasn't sure it was correct.
> 
> When Jim asked me to file this bug, in bug 1238964 comment 7, I think he was
> referring to managing the locking and unlocking of the HGLOBAL.
> 
> Jim - is that correct?
> 
> I wonder if for HGLOBALs it would be good to have something more
> sophisticated than an nsAutoRef class, which managed the allocation/freeing
> and lock/unlock?

The GlobalUnlock function will free the memory even if it's still locked.
And if we have a wrapper class around HGLOBAL, it has to expose lock and unlock
functions, because maybe we use those operations several times in a function.

> Depending on what Jim says this may be irrelevant, but:
> 
> embedding/components/printingui/win/nsPrintDialogUtil.cpp
> >  // NOTE: We only need to free hGlobalDevMode when the dialog is cancelled
> >  // When the user prints, it comes back in the printdlg struct and 
> >  // is used and cleaned up later
> >  hGlobalDevMode = CreateGlobalDevModeAndInit(printerName, aPrintSettings);
> 	
> I'm not entirely sure this comment is true.
> When the user prints I think we copy the DEVMODE into the print settings, so
> I think we might need to free this but don't.

hGlobalDevMode's memory is copied into psWin, and in destructor of nsPrintSettingsWin class,
it is freed, but using ::HeapFree, weird!

> psWin->SetDevMode(devMode); // copies DevMode
> psWin->CopyFromNative(prntdlg.hDC, devMode);
>
> if (mDevMode) ::HeapFree(::GetProcessHeap(), 0, mDevMode);

By the way, I think we can use nsAutoDevMode in function CreateGlobalDevModeAndInit to simplify heap memory management.
Thanks for continuing to look at this, I might be a bit unresponsive for a bit after tomorrow ... on PTO.

(In reply to so61pi from comment #6)

> The GlobalUnlock function will free the memory even if it's still locked.
> And if we have a wrapper class around HGLOBAL, it has to expose lock and
> unlock
> functions, because maybe we use those operations several times in a function.

I assume you meant GlobalFree and yes we'd need Lock/Unlock.
We'll see what Jim thinks.
 
> hGlobalDevMode's memory is copied into psWin, and in destructor of
> nsPrintSettingsWin class,
> it is freed, but using ::HeapFree, weird!

As part of SetDevMode it calls CopyDevMode, which does HeapAlloc then memcpy.

> > if (mDevMode) ::HeapFree(::GetProcessHeap(), 0, mDevMode);
> 
> By the way, I think we can use nsAutoDevMode in function
> CreateGlobalDevModeAndInit to simplify heap memory management.

Looks like it.
(In reply to Bob Owen (:bobowen) (less responsive 11th-18th) from comment #5)
> Comment on attachment 8716990 [details] [diff] [review]
> Create a RAII helper to manage HGLOBALs and use in ShowNativePrintDialog.
> 
> I knew I was going to get into trouble when I set myself as mentor on this,
> I've only recently come across HGLOBALs. :-)
> 
> I think my wording might have been a bit misleading in the description.
> The checking of the freeing comment was a side thing I mentioned, because I
> wasn't sure it was correct.
> 
> When Jim asked me to file this bug, in bug 1238964 comment 7, I think he was
> referring to managing the locking and unlocking of the HGLOBAL.
> 
> Jim - is that correct?
> 
> I wonder if for HGLOBALs it would be good to have something more
> sophisticated than an nsAutoRef class, which managed the allocation/freeing
> and lock/unlock?

Sometimes we have to free these on our side and sometimes we just want to detach before passing it to windows. I think nsAutoRef gives us both of these. 

Locking is a bit different. I'd have to dig into the code to see if we need something specific for that. My original interest was allocation/free of these, but maybe we could add some methods for lock/unlock too. Depends on how useful it would be.
Flags: needinfo?(jmathies)
I have a concern though, maybe it's irrelevant.

In function ShowNativePrintDialog we get a global handle to DevMode (hGlobalDevMode) from CreateGlobalDevModeAndInit, assign it to prntdlg.hDevMode.

Then the memory of DevMode is duplicated in SetDevMode function using HeapAlloc (in CopyDevMode function, as mentioned by Bob Owen).

Until the end, I don't see any deallocation for hGlobalDevMode or prntdlg.hDevMode, except a GlobalUnlock.

Does that mean we have a memory leak here?
Comment on attachment 8716990 [details] [diff] [review]
Create a RAII helper to manage HGLOBALs and use in ShowNativePrintDialog.

Review of attachment 8716990 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry about the delay from my PTO.

I've gone back over this and there are a few nits and mainly ideas for making it even tidier.

Replying to other comments as well ...

(In reply to Jim Mathies [:jimm] from comment #8)
> (In reply to Bob Owen (:bobowen) (less responsive 11th-18th) from comment #5)

> Sometimes we have to free these on our side and sometimes we just want to
> detach before passing it to windows. I think nsAutoRef gives us both of
> these. 

Ah right I was confused, because ScopedHGlobal that you mentioned in bug 1238964 comment 7 only deals with lock/unlock.

But I agree that the allocation/free handling is the most important.

(In reply to so61pi from comment #9)
> I have a concern though, maybe it's irrelevant.
> 
> In function ShowNativePrintDialog we get a global handle to DevMode
> (hGlobalDevMode) from CreateGlobalDevModeAndInit, assign it to
> prntdlg.hDevMode.
> 
> Then the memory of DevMode is duplicated in SetDevMode function using
> HeapAlloc (in CopyDevMode function, as mentioned by Bob Owen).
> 
> Until the end, I don't see any deallocation for hGlobalDevMode or
> prntdlg.hDevMode, except a GlobalUnlock.
> 
> Does that mean we have a memory leak here?

Yeah, this was my point in comment 5, as far as I can see it is a leak.
I just wondered if I was missing something special about HGLOBAL.

::: embedding/components/printingui/win/nsPrintDialogUtil.cpp
@@ +54,5 @@
>  
>  // This is for extending the dialog
>  #include <dlgs.h>
>  
> +#include "nsWindowsHelpers.h"

nit: this is already in the .h file.

@@ +468,2 @@
>  {
> +  nsAutoGlobalMem autoDevMode;

Might want to rename this if we're going to use nsAutoDevMode.

@@ +470,5 @@
>    HANDLE hPrinter = nullptr;
>    // const cast kludge for silly Win32 api's
>    LPWSTR printName = const_cast<wchar_t*>(static_cast<const wchar_t*>(aPrintName.get()));
>    BOOL status = ::OpenPrinterW(printName, &hPrinter, nullptr);
>    if (status) {

We could early return here on !status.

@@ +472,5 @@
>    LPWSTR printName = const_cast<wchar_t*>(static_cast<const wchar_t*>(aPrintName.get()));
>    BOOL status = ::OpenPrinterW(printName, &hPrinter, nullptr);
>    if (status) {
>  
>      LPDEVMODEW  pNewDevMode;

Like you suggest, let's use nsAutoDevMode and move this down to where we use it.

@@ +486,5 @@
>      pNewDevMode = (LPDEVMODEW)::HeapAlloc (::GetProcessHeap(), HEAP_ZERO_MEMORY, dwNeeded);
>      if (!pNewDevMode) return nullptr;
>  
> +    autoDevMode.own((HGLOBAL)::GlobalAlloc(GHND, dwNeeded));
> +    if (!autoDevMode.get()) {

With the early return, we can just move the declaration down to here and use construction instead of own(), I think.

Also, just !autoDevMode will work I think.

@@ +499,1 @@
>        ::ClosePrinter(hPrinter);

Looks like we don't always close this on all paths, maybe another helper would be useful.

@@ +506,1 @@
>      if (devMode) {

Again we could early return on !devMode and let our helpers clean up.

@@ +595,5 @@
>  
>    uint32_t len = printerName.Length();
> +  nsAutoGlobalMem autoDevNames((HGLOBAL)::GlobalAlloc(GHND, sizeof(wchar_t) * (len + 1) + 
> +                                                         sizeof(DEVNAMES)));
> +  if (!autoDevNames.get()) {

nit: trailing whitespace
nit: !autoDevNames

@@ -623,5 @@
>    // The PRINTDLG.hDevMode requires that it be a moveable memory object
> -  // NOTE: We only need to free hGlobalDevMode when the dialog is cancelled
> -  // When the user prints, it comes back in the printdlg struct and 
> -  // is used and cleaned up later
> -  hGlobalDevMode = CreateGlobalDevModeAndInit(printerName, aPrintSettings);

I'm not entirely sure this comment is true.
When the user prints I think we copy the DEVMODE into the print settings, so I think we might need to free this but don't.

@@ +626,5 @@
>  
>    prntdlg.lStructSize = sizeof(prntdlg);
>    prntdlg.hwndOwner   = aHWnd;
> +  prntdlg.hDevMode    = autoDevMode.get();
> +  prntdlg.hDevNames   = autoDevNames.disown();

nit: do we need the get?
Also, should we be disowning autoDevNames?
I can't work out how this is getting freed.

@@ +798,4 @@
>      return NS_ERROR_ABORT;
>    }
>  
> +  autoDevMode.disown();

As discussed, I think this can go.
Attached patch patch_v2.diff (obsolete) — Splinter Review
Thanks for your reply, Bob Owen.

I've created a new patch follow your comment, please review it.

About the inclusion of nsWindowsHelpers.h, I see that nsPrintDialogUtil.cpp doesn't include its header, so I just keep thing as it is. Beside, nsPrintDialogUtil.h doesn't have all of its dependencies (for nsIWebBrowserPrint, nsIPrintSettings...). Should we fix this too?

Another thing is, to create a RAII helper class for hPrinter, I add a new class HPRINTER in nsWindowsHelpers.h, you can read the comment in the patch to see if it's a correct & elegant solution.
Attachment #8716990 - Attachment is obsolete: true
Attachment #8723411 - Flags: review?(bobowen.code)
Comment on attachment 8723411 [details] [diff] [review]
patch_v2.diff

Review of attachment 8723411 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for this, looking good to me, just a couple of nits.

Can you then ask jimm for review, as I'm not a peer of this code.
I'm not actually sure who is a peer, as this code is fairly old and unloved, but jimm has reviewed it before and has more experience with these Windows memory management functions.

One other thing I thought, as I was looking through it, was why we use the separate HeapAlloc()ed DEVMODE at all in CreateGlobalDevModeAndInit, but maybe I'm missing something.

(In reply to so61pi from comment #11)

> About the inclusion of nsWindowsHelpers.h, I see that nsPrintDialogUtil.cpp
> doesn't include its header, so I just keep thing as it is. Beside,
> nsPrintDialogUtil.h doesn't have all of its dependencies (for
> nsIWebBrowserPrint, nsIPrintSettings...). Should we fix this too?

Actually it looks like CreateGlobalDevModeAndInit is only used in nsPrintDialogUtil.cpp now, so we could get rid of this and the #include and make it static in nsPrintDialogUtil.cpp.

We could file a follow-up bug for the dependencies and presumably they could be removed from whatever is including this.
I've found with tidying up this printing code, you have to draw a line somewhere, otherwise it turns into too big a re-write. :-)

> Another thing is, to create a RAII helper class for hPrinter, I add a new
> class HPRINTER in nsWindowsHelpers.h, you can read the comment in the patch
> to see if it's a correct & elegant solution.

I think this is OK, we'll see what jimm thinks.

::: embedding/components/printingui/win/nsPrintDialogUtil.cpp
@@ +470,4 @@
>    // const cast kludge for silly Win32 api's
>    LPWSTR printName = const_cast<wchar_t*>(static_cast<const wchar_t*>(aPrintName.get()));
>    BOOL status = ::OpenPrinterW(printName, &hPrinter, nullptr);
> +  if (!status) return nullptr;

nit: curly braces and separate lines even for single line ifs please.
A few other instances of this.

if (...) {
  ...
}
Attachment #8723411 - Flags: review?(bobowen.code) → feedback+
Assignee: nobody → so61pi.re
Attached patch patch_v3.diff (obsolete) — Splinter Review
Jim, could you review this code, please?

(In reply to Bob Owen (:bobowen) from comment #12)
> One other thing I thought, as I was looking through it, was why we use the
> separate HeapAlloc()ed DEVMODE at all in CreateGlobalDevModeAndInit,
> but maybe I'm missing something.

Maybe CreateGlobalDevModeAndInit only used GlobalAlloc, then someone who updated the code just didn't want to use old Windows API :) .

> We could file a follow-up bug for the dependencies and presumably they
> could be removed from whatever is including this.
> I've found with tidying up this printing code, you have to draw a line somewhere,
> otherwise it turns into too big a re-write. :-)

Yes, it should be in another bug report.
Attachment #8723411 - Attachment is obsolete: true
Attachment #8724086 - Flags: review?(jmathies)
Comment on attachment 8724086 [details] [diff] [review]
patch_v3.diff

Review of attachment 8724086 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry it took so long to get back to this. Looks really good.

::: xpcom/base/nsWindowsHelpers.h
@@ +161,5 @@
> +
> +// because Printer's HANDLE uses ClosePrinter and
> +// we already have nsAutoRef<HANDLE> which uses CloseHandle
> +// so we need to create a wrapper class for HANDLE
> +// to have another specialization for nsAutoRefTraits

nit - lets clean these comments up, make sure they fall around ~80 chars in length.

@@ +182,5 @@
> +private:
> +    HANDLE m_hPrinter;
> +};
> +
> +// winspool.h header has macro AddMonitor, it conflicts with

same here.
Attachment #8724086 - Flags: review?(jmathies) → review+
Attached patch patch_v4.diff (obsolete) — Splinter Review
I've edited the comment.
Attachment #8724086 - Attachment is obsolete: true
Attachment #8737472 - Flags: review?(jmathies)
Attachment #8737472 - Flags: review?(jmathies) → review+
so61pi, can you do a pull in your local repo and merge this patch to mc tip? The patch failed to apply to mc for a try run.
Flags: needinfo?(so61pi.re)
Attached patch patch_v5.diff (obsolete) — Splinter Review
Done!

I forgot about that though, sorry.
Attachment #8737472 - Attachment is obsolete: true
Flags: needinfo?(so61pi.re)
Attachment #8737563 - Flags: review?(jmathies)
(In reply to so61pi from comment #17)
> Created attachment 8737563 [details] [diff] [review]
> patch_v5.diff
> 
> Done!
> 
> I forgot about that though, sorry.

Lets get the meta data for this patch filled in. 

hg qref -m "Bug xxx - description of the work. r=jimm"

trypush:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1729ddd27f7

I did a full mochitest run with windows release and debug.
Comment on attachment 8737563 [details] [diff] [review]
patch_v5.diff

Review of attachment 8737563 [details] [diff] [review]:
-----------------------------------------------------------------

::: embedding/components/printingui/win/nsPrintDialogUtil.cpp
@@ +493,5 @@
> +  if (!globalDevMode) {
> +    return nullptr;
> +  }
> +
> +  DWORD dwRet = ::DocumentPropertiesW(gParentWnd, hPrinter, printName, newDevMode, nullptr, DM_OUT_BUFFER);

Lets limit these lines to 80 chars or less too through wrapping. Might as well clean this code up a bit while we're in here.
Attachment #8737563 - Flags: review?(jmathies) → review+
Attached patch patch_v6.diff (obsolete) — Splinter Review
Aside from all cleanups, I see an error on trypush server complaining about ClosePrinter linkage specification, so I change the ClosePrinter's prototype (in nsWindowsHelpers.h) to

> extern "C" BOOL WINAPI ClosePrinter(HANDLE hPrinter);

to match with the one in winspool.h

Please let me know if it's correct.
Attachment #8737563 - Attachment is obsolete: true
Flags: needinfo?(jmathies)
Attachment #8738044 - Flags: review?(jmathies)
This build locally for me, lets see how another try push comes out - 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c62c1c8c3f8
Flags: needinfo?(jmathies)
Attachment #8738044 - Flags: review?(jmathies) → review+
alright, lets get this landed!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e91dafc1450e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
We might have to back this out or get a quick follow-up.
It is causing a crash now when I try to print while debugging.

It looks like it is using the the release in nsSimpleRef<HANDLE> not nsAutoRefTraits<HGLOBAL>.
As the handle is invalid this causes an exception while debugging and it means that the memory isn't getting freed.

I assume this is down to HGLOBAL just being a typedef of HANDLE.
Flags: needinfo?(so61pi.re)
Backed this out until we can fix the HGLOBAL / HANDLE mix up:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3b202a7788c5
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch patch_v7.diff (obsolete) — Splinter Review
I agree, nsSimpleRef is derived from nsAutoRefTraits, it is the problem.

My solution is adding a new wrapper class for HGLOBAL (called nsHGLOBAL) and make a nsAutoRefTraits specialization for it.

I also change HPRINTER name to nsHPRINTER because I think HPRINTER is confusing.
Attachment #8738044 - Attachment is obsolete: true
Flags: needinfo?(so61pi.re)
Attachment #8739613 - Flags: review?(jmathies)
Attachment #8739613 - Flags: review?(bobowen.code)
Attached patch patch_v7.diff (obsolete) — Splinter Review
Fix a typo, my mistake.
Attachment #8739613 - Attachment is obsolete: true
Attachment #8739613 - Flags: review?(jmathies)
Attachment #8739613 - Flags: review?(bobowen.code)
Attachment #8739617 - Flags: review?(jmathies)
Attachment #8739617 - Flags: review?(bobowen.code)
Comment on attachment 8739617 [details] [diff] [review]
patch_v7.diff

I'm going to defer to bowen since he detected the HANDLE issue while testing.
Attachment #8739617 - Flags: review?(jmathies)
Comment on attachment 8739617 [details] [diff] [review]
patch_v7.diff

Review of attachment 8739617 [details] [diff] [review]:
-----------------------------------------------------------------

This is picking the correct nsAutoRefTraits now.

There's a couple of things I hadn't picked up on before as well.

Because of the interaction with the native dialog this isn't covered by the automated tests, so can you make sure everything is being release as expected manually.

::: embedding/components/printingui/win/nsPrintDialogUtil.cpp
@@ +463,5 @@
>  // NOTE:
>  //   This function assumes that aPrintName has already been converted from 
>  //   unicode
>  //
> +static nsReturnRef<nsHGLOBAL> CreateGlobalDevModeAndInit(

nit: can you wrap after return type and then within the bracket if needed, please.

@@ +472,5 @@
>    // const cast kludge for silly Win32 api's
>    LPWSTR printName = const_cast<wchar_t*>(static_cast<const wchar_t*>(aPrintName.get()));
>    BOOL status = ::OpenPrinterW(printName, &hPrinter, nullptr);
> +  if (!status) {
> +    return nsHGLOBAL(nullptr);

I think all of these should be nsReturnRef<nsHGLOBAL>()

@@ +492,5 @@
> +  if (!newDevMode) {
> +    return nsHGLOBAL(nullptr);
> +  }
> +
> +  nsHGLOBAL hDevMode = (HGLOBAL)::GlobalAlloc(GHND, dwNeeded);

nit: Don't think the cast is needed.

@@ +591,5 @@
>    // Now create a DEVNAMES struct so the the dialog is initialized correctly.
>  
>    uint32_t len = printerName.Length();
> +  nsHGLOBAL hDevNames = (HGLOBAL)::GlobalAlloc(GHND, sizeof(wchar_t) * (len + 1) +
> +                                               sizeof(DEVNAMES));

nit: wrap after the )

@@ +614,5 @@
>    // from the Printer Name
>    // The PRINTDLG.hDevMode requires that it be a moveable memory object
> +  // NOTE: autoDevMode is automatically freed when any error occurred
> +  nsAutoGlobalMem autoDevMode(CreateGlobalDevModeAndInit(printerName, aPrintSettings));
> +  nsHGLOBAL hDevMode = autoDevMode.get();

nit: can we just do the get() below as it's only used once.

::: xpcom/base/nsWindowsHelpers.h
@@ +227,5 @@
> +  {
> +    return m_hGlobal;
> +  }
> +
> +  HGLOBAL* operator&()

Don't think we need this.

@@ +249,5 @@
> +  }
> +
> +  static void Release(RawRef hGlobal)
> +  {
> +    if (hGlobal != Void()) {

This check is already done in nsAutoRefBase::SafeRelease.

I know that the others do as well, I'll file a separate bug for that.

@@ +298,5 @@
> +  }
> +
> +  static void Release(RawRef hPrinter)
> +  {
> +    if (hPrinter != Void()) {

Same here.
Attachment #8739617 - Flags: review?(bobowen.code) → review+
Attached patch patch_v8.diffSplinter Review
I have changed the files according to your comments, please review it.

> Because of the interaction with the native dialog this isn't covered by the 
> automated tests, so can you make sure everything is being release as expected 
> manually.

How can I do this?
Attachment #8739617 - Attachment is obsolete: true
Attachment #8740936 - Flags: review?(bobowen.code)
Comment on attachment 8740936 [details] [diff] [review]
patch_v8.diff

Review of attachment 8740936 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

(In reply to so61pi from comment #32)

> > Because of the interaction with the native dialog this isn't covered by the 
> > automated tests, so can you make sure everything is being release as expected 
> > manually.
> 
> How can I do this?

Debugging with a debug build and making sure the new Release members are called as expected.

Ctrl P - brings up the print dialog, I'd test OK and Cancel.
You can print to XPS to save on paper. :-)
Attachment #8740936 - Flags: review?(bobowen.code) → review+
I've debugged it, and the Release member functions are called correctly for both nsHPRINTER and nsHGLOBAL.

Another thing (unrelated to this bug) I saw while debugging is the returned value of DocumentPropertiesW is handled incorrectly in CreateGlobalDevModeAndInit function. DocumentPropertiesW 's return type is LONG, but it is casted to DWORD, which changes its sign-ness. This could lead to an error when DocumentPropertiesW returns a negative number.

I think we should file another bug for it.
(In reply to so61pi from comment #34)
> I've debugged it, and the Release member functions are called correctly for
> both nsHPRINTER and nsHGLOBAL.

Thanks, I'll get this landed.
Quick try push to check, don't think it's worth running tests as this code doesn't get exercised.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b58a13293860

> Another thing (unrelated to this bug) I saw while debugging is the returned
> value of DocumentPropertiesW is handled incorrectly in
> CreateGlobalDevModeAndInit function. DocumentPropertiesW 's return type is
> LONG, but it is casted to DWORD, which changes its sign-ness. This could
> lead to an error when DocumentPropertiesW returns a negative number.
> 
> I think we should file another bug for it.

Well spotted, yes please file a bug for this.
https://hg.mozilla.org/mozilla-central/rev/72e36db09b3a
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.