Closed Bug 1330755 Opened 3 years ago Closed 3 years ago

nsWindow has improper use of nsAutoHDC

Categories

(Core :: Widget: Win32, defect)

53 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(2 files)

At [1] 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 [3] which explicitly say that you should ::ReleaseDC the handle [3] and not ::DeleteDC it [4].

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.

[1] http://searchfox.org/mozilla-central/rev/3f614bdf91a2379a3e2c822da84e524f5e742121/widget/windows/nsWindow.cpp#1396
[2] http://searchfox.org/mozilla-central/rev/3f614bdf91a2379a3e2c822da84e524f5e742121/xpcom/base/nsWindowsHelpers.h#66
[3] https://msdn.microsoft.com/en-us/library/windows/desktop/dd144871(v=vs.85).aspx
[4] https://msdn.microsoft.com/en-us/library/windows/desktop/dd183533(v=vs.85).aspx
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 kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eca7348f32d8
Properly release the window device context. r=jimm
https://hg.mozilla.org/mozilla-central/rev/eca7348f32d8
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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:kats@mozilla.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.
Flags: needinfo?(bugmail)
Attached patch Follow-upSplinter Review
Per previous comments.
Attachment #8827480 - Flags: review?(bobowencode)
Attachment #8827480 - Flags: review?(bobowencode) → review+
Pushed by kgupta@mozilla.com:
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 → ---
https://hg.mozilla.org/mozilla-central/rev/529788a8225a
Status: REOPENED → RESOLVED
Closed: 3 years ago3 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.