Uninitialised value use in nsLayoutUtils::TransformToAncestorAndCombineRegions

RESOLVED FIXED in Firefox 51

Status

()

Core
Layout
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jseward, Unassigned)

Tracking

48 Branch
mozilla51
Points:
---

Firefox Tracking Flags

(firefox48 wontfix, firefox49 wontfix, firefox50 wontfix, firefox51 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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 8785288 [details]
Valgrind complaint
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 [1] 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?

[1] http://searchfox.org/mozilla-central/rev/f80822840bc5f3d2d3cae3ece621ddbce72e7f54/layout/base/nsLayoutUtils.cpp#2903
Blocks: 1271432
status-firefox48: --- → wontfix
status-firefox49: --- → wontfix
status-firefox50: --- → fix-optional
Version: Trunk → 48 Branch
Created attachment 8785927 [details] [diff] [review]
Revised as per comment 4

Yes, that also seems to fix the problem.
Attachment #8785290 - Attachment is obsolete: true
Attachment #8785927 - Flags: review?(bugmail)
Comment on attachment 8785927 [details] [diff] [review]
Revised as per comment 4

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

Thanks!
Attachment #8785927 - Flags: review?(bugmail) → review+

Comment 7

a year ago
Pushed by jseward@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d9cabea1e56
Uninitialised value use in nsLayoutUtils::TransformToAncestorAndCombineRegions.  r=kats.

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3d9cabea1e56
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
:kats, you marked this "status-firefox50: fix-optional" .. is there anything
that should happen here?
Flags: needinfo?(bugmail)
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.
status-firefox50: fix-optional → wontfix
Flags: needinfo?(bugmail)
I'm fine with leaving it as it is now.
You need to log in before you can comment on or make changes to this bug.