549779 - valgrind: nsMenuPopupFrame::AdjustPositionForAnchorAlign creates undefined nsRects

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Julian Seward [:jseward]]

There is a path through layout/xul/base/source/nsMenuPopupFrame.cpp
function nsMenuPopupFrame::AdjustPositionForAnchorAlign which causes
it to return an undefined nsPoint (stack-allocated garbage).  It's
actually very simple.  We have:

783:  nsPoint pnt;
784:  switch (popupAnchor) {
785:    case POPUPALIGNMENT_TOPLEFT:
786:      pnt = anchorRect.TopLeft();
787:      break;
788:    case POPUPALIGNMENT_TOPRIGHT:
789:      pnt = anchorRect.TopRight();
790:      break;
791:    case POPUPALIGNMENT_BOTTOMLEFT:
792:      pnt = anchorRect.BottomLeft();
793:      break;
794:    case POPUPALIGNMENT_BOTTOMRIGHT:
795:      pnt = anchorRect.BottomRight();
796:      break;
797:  }

It then goes on to possibly add/subtract various offsets to pnt,
and then returns it.  That's all well and good, but the problem
is the switch doesn't handle the case POPUPALIGNMENT_NONE, in
which case garbage will be returned.  Adding the case, and a
printf, shows this case does indeed occur.

The bogus nsPoint is returned to the caller, nsMenuPopupFrame::SetPopupPosition,
which eventually hands it onwards via a call to nsMenuPopupFrame::FlipOrResize,
and, I think, various other parts of the graphics system.  Valgrind
tracks it as causing an immense number of errors, 49762 errors from 497 contexts
(different stack tracebacks) in this test case.  These are shown in
the attached file.

The flaw is simple once you see it, but tracking it down from the
presented information proved very difficult.


HOW TO REPRO: run layout/base/tests/test_bug420499.xul
on Memcheck as described in bug 549224.

Comment 1

[Julian Seward [:jseward]]

Attachment: list of resulting valgrind errors

Note that the origin of the undefined value is erroneously listed
as being from 
nsMenuPopupFrame::SetPopupPosition(nsIFrame*, int) (nsMenuPopupFrame.cpp:908)
whereas it is really from 
nsMenuPopupFrame::AdjustPositionForAnchorAlign(const nsRect& anchorRect,
                                               PRBool& aHFlip, PRBool& aVFlip)
as discussed above.  I think this happens because the latter got inlined
into the former.

Comment 2

[Julian Seward [:jseward]]

Attachment: proposed fix

Proposed fix.  Obviously the minimal safe thing to do is give pnt
an initial value of (0,0) so its safe regardless of whatever else
happens.  I took the liberty of adding do-nothing cases for
POPUPALIGNMENT_NONE to show that they are deliberately no-ops
rather than were merely forgotten about.

Comment 3

[Markus Stange [:mstange]]

You can request review from enndeakin.

Comment 4

[Neil Deakin]

Actually this is fixed by the patch in bug 524545, which has some problems with the test on Mac which I've never figured out. Two options here:

- check in the patch in bug 524545 with the test disabled on Mac
- check in just something like the patch in this bug, however POPUPALIGNMENT_NONE should behave the same as POPUPALIGNMENT_TOPLEFT. The patch in bug 524545 uses a switch default for this.

Comment 5

[Nicholas Nethercote [inactive]]

Two thumbs up for always including 'default' cases in switch statements!

Comment 6

[Julian Seward [:jseward]]

Attachment: patch v2

patch revised as per comment 5

Comment 7

[Julian Seward [:jseward]]

So much for guessing comment numbers.  That should have been "patch
revised as per comment 4."

Comment 8

[Neil Deakin]

The intent was to just add a 'default' case, and rely on it for the none value. Really, just the first part of https://bug524545.bugzilla.mozilla.org/attachment.cgi?id=408843

Comment 9

[Julian Seward [:jseward]]

Attachment: patch v3, in accordance with comment 8

Comment 10

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/581260c2e0d7