STR: DISPLAY=:1.0 G_SLICE=always-malloc ./mach mochitest -f plain \ --keep-open=no --valgrind=vMOZHX \ --valgrind-args=--num-transtab-sectors=40,--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,--error-limit=no,--freelist-vol=500000000,--redzone-size=64,--num-callers=20,--leak-check=summary,--track-origins=no \ gfx/layers/apz/test/mochitest/test_group_touchevents.html produces the error shown in the attachment, plus more depending on the optimization level of the build. The cause (as it seems to me) is as follows: function nsLayoutUtils::TransformToAncestorAndCombineRegions at layout/base/nsLayoutUtils.cpp:9096 9096 bool isPrecise; This is used to receive a result from the call 9099 nsRect transformed = TransformFrameRectToAncestor( 9100 aFrame, it.Get(), aAncestorFrame, &isPrecise, aMatrixCache); nsLayoutUtils::TransformFrameRectToAncestor calls onwards, on some paths, to TransformGfxRectToAncestor, assuming that that in turn will assign via the bool* out-parameter -- here called aPreservesAxisAlignedRectangles. But that assumption isn't valid. TransformGfxRectToAncestor doesn't do that, in the case commented "// We are given a matrix to use, so use it" and so these two functions return, |isPrecise| still has no value, but is used anyway: 9103 nsRegion* dest = isPrecise ? aPreciseTargetDest : aImpreciseTargetDest;
Created attachment 8785290 [details] [diff] [review] bug1298377-1.cset Writes to *aPreservesAxisAlignedRectangles on all paths through TransformGfxRectToAncestor.
Possibly related to bug 1271432.
I think in practice we never trigger the uninitialized value use, because in order to go down the "// We are given a matrix to use, so use it" codepath, we have to have previously gone down the "else" clause of that statement, since that's the only thing that populates aMatrixCache. And that else clause will have put some value into *aPreservesAxisAlignedRectangles. However, to guard against future changes, and to quiet the Valgrind complaint, we could move the "if (aPreservesAxisAlignedRectangles)" at  down to be outside the if/else statement, and so it would get run in both branches. It will cause some extra work but shouldn't be too bad. Can you try that and confirm it satisfied the Valgrind complaint?  http://searchfox.org/mozilla-central/rev/f80822840bc5f3d2d3cae3ece621ddbce72e7f54/layout/base/nsLayoutUtils.cpp#2903
Created attachment 8785927 [details] [diff] [review] Revised as per comment 4 Yes, that also seems to fix the problem.
Comment on attachment 8785927 [details] [diff] [review] Revised as per comment 4 Review of attachment 8785927 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/3d9cabea1e56 Uninitialised value use in nsLayoutUtils::TransformToAncestorAndCombineRegions. r=kats.
:kats, you marked this "status-firefox50: fix-optional" .. is there anything that should happen here?
You can request uplift of the patch to 50 if you want. Although really since we should never actually use the uninitialized value in current code it's probably not worth it. I'll mark it as wontfix for 50 instead.
I'm fine with leaving it as it is now.