Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Description • 8 years ago

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

	Jim Mathies [:jimm]
	Comment 2 • 8 years ago
mozreview-review

Comment on attachment 8826344 [details]
Bug 1330755 - Properly release the window device context.

https://reviewboard.mozilla.org/r/104276/#review105032

thanks!

	Pulsebot
	Comment 3 • 8 years ago

Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eca7348f32d8
Properly release the window device context. r=jimm

	Carsten Book [:Tomcat]
	Comment 4 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/eca7348f32d8

	Sylvestre Ledru [:Sylvestre]
	Comment 5 • 8 years ago

is it something you want to uplift to aurora? Thanks
(too late for 51)

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 6 • 8 years ago

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?

	Bob Owen (:bobowen)
	Comment 7 • 8 years ago

(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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 8 • 8 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 9 • 8 years ago

Per previous comments.

	Pulsebot
	Comment 10 • 8 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 11 • 8 years ago

Reopening until the second patches reaches m-c.

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/529788a8225a

	Bob Owen (:bobowen)
	Comment 13 • 8 years ago

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.

	Bob Owen (:bobowen)
	Comment 14 • 8 years ago

Comment on attachment 8827480 [details] [diff] [review]
Follow-up

See comment 13.

	Julien Cristau [:jcristau]
	Comment 15 • 8 years ago

Comment on attachment 8826344 [details]
Bug 1330755 - Properly release the window device context.

device context release fix, aurora52+

	Julien Cristau [:jcristau]
	Comment 16 • 8 years ago

Comment on attachment 8827480 [details] [diff] [review]
Follow-up

device context release fix, aurora52+

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/69f54dcd72c6
https://hg.mozilla.org/releases/mozilla-aurora/rev/3c1334c6b10e