Closed Bug 1296678 Opened 3 years ago Closed 3 years ago
Uninitialised value use in ns
Display Background Image::Is Non Empty Fixed Image
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)
(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.
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 email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/137be83b273c Uninitialised value use in nsDisplayBackgroundImage::IsNonEmptyFixedImage. r=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 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.