940488 - RotatedBuffer::mDidSelfCopy is uninitialized

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

When the in-place buffer un-rotation was landed in bug 921212, a new variable called mDidSelfCopy was introduced.  It appears, however, that the constructors for RotatedBuffer do not initialize the boolean.

This could lead to us perform unnecessary copies in ContentClientDoubleBuffered::SyncFrontBufferToBackBuffer.

It seems possible that this might also cause us to present the wrong content since we end up doing this:

    mBufferRect.MoveTo(mFrontBufferRect.TopLeft());

Instead of:

    mBufferRect = mFrontBufferRect;

If we are silently eating frames, could this explain the FPS regression reports?

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Initialize RotatedBuffer::mDidSelfCopy

Comment 2

[Benoit Girard (:BenWa)]

Comment on attachment 8334641 [details] [diff] [review]
Initialize RotatedBuffer::mDidSelfCopy

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

Patch looks good. IMO we should uplift this to 1.2.

Any idea why the compiler isn't warning here?

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Benoit Girard (:BenWa) from comment #2)
> Any idea why the compiler isn't warning here?

From the IRC discussion, this is covered by -Weffc++ flag.  Apparently we use -Wall which does not include -Weffc++.  Searching online indicates there may be issues with false positives from this flag.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Initalize RotatedBuffer::mDidSelfCopy

Update patch title to reflect reviewer.  Carry r+ forward.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/59994113d911

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/59994113d911

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/df709dd6cf3f

Assuming this is B2G-only and therefore doesn't need uplift to Aurora/Beta.

Comment 8

[Benoit Girard (:BenWa)]

Other platforms could be running into this. It's an uninitialized variable so this is a simple patch that isn't likely to regress. I'd vote for uplift everywhere given the loss risk, high benefit ratio.