Closed Bug 1330755 Opened 3 years ago Closed 3 years ago
Window has improper use of ns Auto HDC
At  a nsAutoHDC is used to wrap the device context provided by ::GetDC. However nsAutoHDC calls ::DeleteDC in its Release() function, which is inappropriate for a device context provided by ::GetDC. See MSDN docs  which explicitly say that you should ::ReleaseDC the handle  and not ::DeleteDC it . The other use site of nsAutoHDC is correct, because it wraps a dc provided by ::CreateICW which *is* supposed to be freed by ::DeleteDC. Likewise, all other call sites of ::GetDC properly release it using ::ReleaseDC.  http://searchfox.org/mozilla-central/rev/3f614bdf91a2379a3e2c822da84e524f5e742121/widget/windows/nsWindow.cpp#1396  http://searchfox.org/mozilla-central/rev/3f614bdf91a2379a3e2c822da84e524f5e742121/xpcom/base/nsWindowsHelpers.h#66  https://msdn.microsoft.com/en-us/library/windows/desktop/dd144871(v=vs.85).aspx  https://msdn.microsoft.com/en-us/library/windows/desktop/dd183533(v=vs.85).aspx
3 years ago
3 years ago
Assignee: nobody → bugmail
Comment on attachment 8826344 [details] Bug 1330755 - Properly release the window device context. https://reviewboard.mozilla.org/r/104276/#review105032 thanks!
Attachment #8826344 - Flags: review?(jmathies) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/eca7348f32d8 Properly release the window device context. r=jimm
is it something you want to uplift to aurora? Thanks (too late for 51)
To be honest I don't know when the CreateScrollSnapshot function is invoked or what effect this might have on the user experience (if any). Bob, do you think this is worth uplifting, and if so, would you mind filling in the uplift request form?
Flags: needinfo?(bugmail) → needinfo?(bobowencode)
(In reply to Kartikaya Gupta (email:email@example.com) from comment #6) > To be honest I don't know when the CreateScrollSnapshot function is invoked > or what effect this might have on the user experience (if any). Bob, do you > think this is worth uplifting, and if so, would you mind filling in the > uplift request form? It's used at the start of a scroll in the main process when we have windowed plugins. (It used to happen at the end of the scroll in the content process, which is where I moved this code from, because it broke with sandboxing.) I think we should uplift it and I don't mind at all, thanks for fixing it. I've got the request text ready, but I just noticed that perhaps the MakeScopeExit should be after the null check or have it's own null check in the lambda. Not sure if it matters if we call ReleaseDC with a null HDC.
Flags: needinfo?(bobowencode) → needinfo?(bugmail)
That's a good point. I'm also not sure what ReleaseDC does if called with a null HDC, so it's probably safer to just move the MakeScopeExit down a few lines to be after the null check.
Per previous comments.
Attachment #8827480 - Flags: review?(bobowencode)
Attachment #8827480 - Flags: review?(bobowencode) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/529788a8225a Follow-up to move the MakeScopeExit to after the null check. r=bobowen
Reopening until the second patches reaches m-c.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 3 years ago → 3 years ago
Resolution: --- → FIXED
Comment on attachment 8826344 [details] Bug 1330755 - Properly release the window device context. Sheriff note: there was an initial patch and follow-up that require uplift. https://hg.mozilla.org/mozilla-central/rev/eca7348f32d8 https://hg.mozilla.org/mozilla-central/rev/529788a8225a Approval Request Comment [Feature/Bug causing the regression]: Bug 1232181, then in bug 1252877. [User impact if declined]: Incorrect deleting instead of releasing native device contexts could cause stability issues. [Is this code covered by automated tests?]: Plugins and scrolling have tests, not sure if this particular code path is hit during tests. [Has the fix been verified in Nightly?]: It is a code correctness fix, so can't be directly verified in Nightly. [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: No. [Why is the change risky/not risky?]: Very simple change that releases the HDC correctly, so should only remove any existing risks that may be causing. [String changes made/needed]: None.
Attachment #8826344 - Flags: approval-mozilla-aurora?
Comment on attachment 8827480 [details] [diff] [review] Follow-up See comment 13.
Attachment #8827480 - Flags: approval-mozilla-aurora?
Comment on attachment 8826344 [details] Bug 1330755 - Properly release the window device context. device context release fix, aurora52+
Attachment #8826344 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8827480 [details] [diff] [review] Follow-up device context release fix, aurora52+
Attachment #8827480 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.