1296678 - Uninitialised value use in nsDisplayBackgroundImage::IsNonEmptyFixedImage

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Julian Seward [:jseward]]

Attachment: Valgrind complaint (one of several)

DISPLAY=:1.0 G_SLICE=always-malloc ./mach mochitest --valgrind=vTRUNK \
  --valgrind-args=--num-transtab-sectors=24,--px-default=allregs-at-mem-access,--px-file-backed=unwindregs-at-mem-access,--fair-sched=yes,--fullpath-after=/MOZ/,--trace-children=yes,--show-mismatched-frees=no,--track-origins=yes \
   browser/base/content/test/general/browser_bug422590.js \
   2>&1 | tee spew-14-05-mc

Produces uninitialised value errors as shown in the attachment.
This happens for several tests in Mochitests.

It appears that nsCSSRendering::PrepareImageLayer can return without
assigning any value to its out-parameter |bool* aOutIsTransformedFixed|
and that is the cause of the problem.

Possibly related to bug 735857.

Comment 1

[Julian Seward [:jseward]]

Attachment: bug1296678-1.cset

Ensure *aOutIsTransformedFixed is given a value on all return paths.

Comment 2

[Botond Ballo [:botond]]

(In reply to Julian Seward [:jseward] from comment #0)
> It appears that nsCSSRendering::PrepareImageLayer can return without
> assigning any value to its out-parameter |bool* aOutIsTransformedFixed|
> and that is the cause of the problem.
> 
> Possibly related to bug 735857.

Yeah, this was introduced in bug 735857.

Comment 3

[Julian Seward [:jseward]]

Note that bug 735857 was uplifted to 48.

Comment 4

[Botond Ballo [:botond]]

(In reply to Julian Seward [:jseward] from comment #3)
> Note that bug 735857 was uplifted to 48.

I had a look at the code to see what the consequences of the uninitialized value could be.

It seems like a background-attachment:fixed image could incorrectly be treated as background-attachment:scroll in the following combination of circumstances:

  - The uninitialized variable |isTransformedFixed| in the nsDisplayBackgroundImage 
    constructor happens to have the initial value of |true|.

  - The early-return path in PrepareImageLayer() is hit, which means "There's no
    image or it's not ready to be painted."

  - The background image is not, in fact, inside a transform.

The treatment would be corrected on a subsequent paint when the image becomes ready to be painted.

I believe this isn't important enough to try getting it into a 48 point-release. It does make sense to uplift it as far as 49, as the patch is very low-risk.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/137be83b273c
Uninitialised value use in nsDisplayBackgroundImage::IsNonEmptyFixedImage.  r=botond.

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/137be83b273c

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 8782968 [details] [diff] [review]
bug1296678-1.cset

Requesting uplift as per comment 4:

Approval Request Comment
[Feature/regressing bug #]:
  Bug 735857

[User impact if declined]:
  A background image which is background-attachment:scroll could
  be treated as background-attachment:fixed by our layout system,
  but only while the image isn't loaded yet.

  Markus and I do not believe this can result in any user-visible
  misbehaviour. However, since the fix is so trivial, I am
  requesting uplift anyways.

[Describe test coverage new/current, TreeHerder]:
  On m-c since 2016-08-26

[Risks and why]: 
  Very low. The patch just ensures that a variable isn't left
  uninitialized during an early-return path of a function.

[String/UUID change made/needed]:
  None

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8782968 [details] [diff] [review]
bug1296678-1.cset

Fixes a mochitest failure, Aurora50+

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/42711ffd066b

Comment 10

[Liz Henry (:lizzard)]

Comment on attachment 8782968 [details] [diff] [review]
bug1296678-1.cset

Regression from 48, seems pretty safe. Taking this for beta 9.

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/bc17999985e5