Closed Bug 1238753 Opened 4 years ago Closed 4 years ago

OS X BasicCompositor can draw uninitialized memory during window resizes

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla46
Tracking Status
firefox45 --- verified
firefox46 --- verified

People

(Reporter: mstange, Assigned: mstange)

References

Details

Attachments

(3 files)

There are two bugs here.

The first one is caused by BasicCompositor not recompositing the whole window when the RectTextureImage's buffer size changes, so we show uninitialized memory from the parts of the new buffer that haven't been drawn to.
The second one is that we're sometimes skipping composition entirely if the invalid region is empty. In that case, since the GL context has been resized, the GL context will be empty and the garbage from the regular window buffer is shown.
This improves upload performance and lets us override the invalid region if we need more than what BasicCompositor thinks we need.

Review commit: https://reviewboard.mozilla.org/r/30411/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30411/
Attachment #8706624 - Flags: review?(matt.woodrow)
Sometimes we need a composition even if nothing has changed, for example for window resizes on Mac.
LayerManagerComposite::UpdateAndRender() already has a check that skips the call to BasicCompositor
if it's not necessary.

Review commit: https://reviewboard.mozilla.org/r/30413/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30413/
Attachment #8706625 - Flags: review?(matt.woodrow)
Comment on attachment 8706623 [details]
MozReview Request: Bug 1238753 - Make BasicCompositor respect changes to mInvalidRegion through StartRemoteDrawingWithRegion properly. r?mattwoodrow

https://reviewboard.mozilla.org/r/30409/#review27123

::: gfx/layers/basic/BasicCompositor.cpp:543
(Diff revision 1)
> +    mInvalidRect = mInvalidRegion.GetBounds();

Maybe add a comment that StartRemoteDrawingInRegion can modify mInvalidRegion?
Attachment #8706623 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8706624 [details]
MozReview Request: Bug 1238753 - Use StartRemoteDrawingInRegion on Mac. r?mattwoodrow

https://reviewboard.mozilla.org/r/30411/#review27125
Attachment #8706624 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8706625 [details]
MozReview Request: Bug 1238753 - Don't skip the call to StartRemoteDrawing in from BasicCompositor::BeginFrame if the invalid region is empty. r?mattwoodrow

https://reviewboard.mozilla.org/r/30413/#review27131
Attachment #8706625 - Flags: review?(matt.woodrow) → review+
Depends on: 1242293
Comment on attachment 8706623 [details]
MozReview Request: Bug 1238753 - Make BasicCompositor respect changes to mInvalidRegion through StartRemoteDrawingWithRegion properly. r?mattwoodrow

Approval Request Comment
[Feature/regressing bug #]: BasicCompositor on OS X
[User impact if declined]: very ugly artifacts when resizing windows if hardware acceleration is off
[Describe test coverage new/current, TreeHerder]: none (we don't run any tests with hardware acceleration disabled on OS X)
[Risks and why]: low, has baked on trunk for almost two weeks
[String/UUID change made/needed]: none

There's a small, mostly harmless (unless you're running a debug build) bug in these patches that's getting fixed in bug 1242293, so we should uplift that one together with these patches once it has landed.
Attachment #8706623 - Flags: approval-mozilla-aurora?
Comment on attachment 8706624 [details]
MozReview Request: Bug 1238753 - Use StartRemoteDrawingInRegion on Mac. r?mattwoodrow

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:
Attachment #8706624 - Flags: approval-mozilla-aurora?
Attachment #8706625 - Flags: approval-mozilla-aurora?
Comment on attachment 8706623 [details]
MozReview Request: Bug 1238753 - Make BasicCompositor respect changes to mInvalidRegion through StartRemoteDrawingWithRegion properly. r?mattwoodrow

Switching the approval request to beta, since this seems to be about uplift to 45.
Attachment #8706623 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Attachment #8706624 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Attachment #8706625 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Comment on attachment 8706623 [details]
MozReview Request: Bug 1238753 - Make BasicCompositor respect changes to mInvalidRegion through StartRemoteDrawingWithRegion properly. r?mattwoodrow


Fix an important graphic issue, taking it.

Should be in 45 beta 2.
Attachment #8706623 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8706624 [details]
MozReview Request: Bug 1238753 - Use StartRemoteDrawingInRegion on Mac. r?mattwoodrow


Fix an important graphic issue, taking it.

Should be in 45 beta 2.
Attachment #8706624 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Attachment #8706625 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
There were no artifacts noticed while resizing windows (with hardware acceleration disabled) on Mac OS X 10.9.5.
Testing was performed using the following Fx versions:
* 45.0b8 - buildID 20160221141421
* 46.0a2 - buildID 20160222004010
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.