Closed Bug 1296678 Opened 3 years ago Closed 3 years ago

Uninitialised value use in nsDisplayBackgroundImage::IsNonEmptyFixedImage

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox48 --- wontfix
firefox49 --- fixed
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: jseward, Unassigned)

References

Details

Attachments

(2 files)

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.
Ensure *aOutIsTransformedFixed is given a value on all return paths.
Attachment #8782968 - Flags: review?(botond)
Attachment #8782968 - Flags: review?(botond) → review+
(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.
Blocks: 735857
Note that bug 735857 was uplifted to 48.
(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.
Pushed by jseward@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/137be83b273c
Uninitialised value use in nsDisplayBackgroundImage::IsNonEmptyFixedImage.  r=botond.
https://hg.mozilla.org/mozilla-central/rev/137be83b273c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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
Attachment #8782968 - Flags: approval-mozilla-beta?
Attachment #8782968 - Flags: approval-mozilla-aurora?
Comment on attachment 8782968 [details] [diff] [review]
bug1296678-1.cset

Fixes a mochitest failure, Aurora50+
Attachment #8782968 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8782968 [details] [diff] [review]
bug1296678-1.cset

Regression from 48, seems pretty safe. Taking this for beta 9.
Attachment #8782968 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.