Closed Bug 1264246 Opened 8 years ago Closed 8 years ago

Uninitialised value use in IsEmpty (gfx/2d/BaseRect.h:61)

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jseward, Assigned: lsalzman)

Details

Attachments

(3 files)

DISPLAY=:1.0 ./mach mochitest \
  --valgrind-args=--show-mismatched-frees=no,--fullpath-after=REL/ \
  --valgrind=/home/sewardj/VgTRUNK/merge/Inst/bin/valgrind \
  dom/canvas/test/test_canvas.html 2>&1 | tee spew-05-mc-11

SkCanvas::getClipBounds is inconsistent in its return conventions.  It
returns a boolean indicating whether or not it succeeded, and it
appears that the original plan was for it to set the out-parameter
|bounds| to a safe value even on failure, so that callers can ignore
the boolean if they want.  But the first return point from the
function doesn't do that: it returns |false| without changing the
out-parameter.

The result is that "Rect GetClipBounds(SkCanvas *aCanvas)" in
DrawTargetSkia.cpp can pass stack-allocated junk back to its caller:

  static inline Rect
  GetClipBounds(SkCanvas *aCanvas)
  {
    SkRect clipBounds;
    aCanvas->getClipBounds(&clipBounds);
    return SkRectToRect(clipBounds);
  }

The uninitialised values get passed around a bit and Valgrind
eventually comments on them (somewhat confusingly) in
BaseRect::IsEmpty (BaseRect.h:61).
Attached file Valgrind complaint
A possible fix.
(In reply to Julian Seward [:jseward] from comment #2)
> Created attachment 8740890 [details] [diff] [review]
> bug1264246-IsEmpty-1.cset
> 
> A possible fix.

We shouldn't really be modifying Skia for something that is better fixed in our own code.
This makes GetClipBounds check the result of SkCanvas::getClipBounds, and if it fails return an empty rect like it should be.

Also this deals with an incidental finding that getClipBounds is actually doing quite expensive math, so only calls it in the case we need to check it against mask bounds.
Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
Attachment #8741033 - Flags: review?(mchang)
Attachment #8741033 - Flags: review?(mchang) → review+
https://hg.mozilla.org/mozilla-central/rev/1442d37b6449
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.