Last Comment Bug 543154 - Uninitialised value use in nsNativeThemeWin::ClassicDrawWidgetBackground
: Uninitialised value use in nsNativeThemeWin::ClassicDrawWidgetBackground
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Other Branch
: x86 Windows XP
: -- normal (vote)
: mozilla7
Assigned To: Jim Mathies [:jimm]
:
: Jim Mathies [:jimm]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-29 18:33 PST by Julian Seward [:jseward]
Modified: 2011-05-25 00:32 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
a fix (1.03 KB, patch)
2010-11-22 10:03 PST, Julian Seward [:jseward]
no flags Details | Diff | Splinter Review
fix (674 bytes, patch)
2011-05-24 11:51 PDT, Jim Mathies [:jimm]
roc: review+
Details | Diff | Splinter Review

Description Julian Seward [:jseward] 2010-01-29 18:33:48 PST
It appears that nsNativeThemeWin::ClassicGetThemePartAndState
                   ( nsIFrame* aFrame, PRUint8 aWidgetType,
                     PRInt32& aPart, PRInt32& aState, PRBool& aFocused )

in (m-c)/widget/src/windows/nsNativeThemeWin.cpp

does not always write a value to 'aFocused'.  This leads to an
uninitialised value use in the caller of said routine, namely
nsNativeThemeWin::ClassicDrawWidgetBackground, viz:

Conditional jump or move depends on uninitialised value(s)
   at 0x10E22655: nsNativeThemeWin::ClassicDrawWidgetBackground
                  (nsnativethemewin.cpp:2584)
   by 0x10E1F02A: nsNativeThemeWin::DrawWidgetBackground
                  (nsnativethemewin.cpp:988)
   by 0x10600299: nsCSSRendering::PaintBackgroundWithSC
                  (nscssrendering.cpp:2088)
   by 0x105FE9CF: nsCSSRendering::PaintBackground (nscssrendering.cpp:1416)
   by 0x106B2A78: nsDisplayBackground::Paint (nsdisplaylist.cpp:711)
   by 0x106B20A8: nsDisplayList::Paint (nsdisplaylist.cpp:415)
   by 0x106B3959: nsDisplayWrapList::Paint (nsdisplaylist.cpp:999)
   by 0x106B441B: nsDisplayClip::Paint (nsdisplaylist.cpp:1195)
   by 0x106B20A8: nsDisplayList::Paint (nsdisplaylist.cpp:415)
   by 0x106DA9A8: nsLayoutUtils::PaintFrame (nslayoututils.cpp:1195)
   by 0x1035E23A: PresShell::Paint (nspresshell.cpp:5776)
   by 0x104757E0: nsViewManager::RenderViews (nsviewmanager.cpp:497)
 Uninitialised value was created by a stack allocation
   at 0x10E22361: nsNativeThemeWin::ClassicDrawWidgetBackground
                  (nsnativethemewin.cpp:2521)


Analysis is as follows.  All line numbers are in nsNativeThemeWin.cpp.

In nsNativeThemeWin::ClassicDrawWidgetBackground we have

2522:  PRInt32 part, state;
2523:  PRBool focused;
2524:  nsresult rv;
2525:  rv = ClassicGetThemePartAndState(aFrame, aWidgetType, part, state, focused);

Apparently it is intended to convert 'aWidgetType' (and possibly
'aFrame') into values for 'part', 'state' and 'focused' by
calling ClassicGetThemePartAndState.  Later in this same function
we have:

2522:  switch (aWidgetType) { 
       [...]
2565:    // Draw controls supported by DrawFrameControl
2566:    case NS_THEME_CHECKBOX:
2567:    case NS_THEME_RADIO:
2568:    case NS_THEME_SCROLLBAR_BUTTON_UP:
2569:    case NS_THEME_SCROLLBAR_BUTTON_DOWN:
2570:    case NS_THEME_SCROLLBAR_BUTTON_LEFT:
2571:    case NS_THEME_SCROLLBAR_BUTTON_RIGHT:
2572:    case NS_THEME_SPINNER_UP_BUTTON:
2573:    case NS_THEME_SPINNER_DOWN_BUTTON:
2574:    case NS_THEME_DROPDOWN_BUTTON:
2575:    case NS_THEME_RESIZER: {
2576:      PRInt32 oldTA;
2577:      // setup DC to make DrawFrameControl draw correctly
2578:      oldTA = ::SetTextAlign(hdc, TA_TOP | TA_LEFT | TA_NOUPDATECP);
2579:      ::DrawFrameControl(hdc, &widgetRect, part, state);
2580:      ::SetTextAlign(hdc, oldTA);
2581:
2582:      // Draw focus rectangles for HTML checkboxes and radio buttons
2583:      // XXX it'd be nice to draw these outside of the frame
2584:      if (focused && (aWidgetType == NS_THEME_CHECKBOX || aWidgetType == NS_THEME_RADIO)) {

The error occurs at 2584, so the culprit must be 'focused' or
'aWidgetType'.  It can't be the latter since V would have
complained at 2522, so it must be the former.

From inspection of ClassicGetThemePartAndState it's clear that
refparam 'aFocused' is unset on most of the paths through.  AFAICS
it only gets set for aWidgetType being on of

   NS_THEME_BUTTON
   NS_THEME_CHECKBOX:
   NS_THEME_RADIO:

That means, of the cases shown above at lines 2566 - 2575, when
aWidgetType has any of these values

   NS_THEME_SCROLLBAR_BUTTON_UP
   NS_THEME_SCROLLBAR_BUTTON_DOWN
   NS_THEME_SCROLLBAR_BUTTON_LEFT
   NS_THEME_SCROLLBAR_BUTTON_RIGHT
   NS_THEME_SPINNER_UP_BUTTON
   NS_THEME_SPINNER_DOWN_BUTTON
   NS_THEME_DROPDOWN_BUTTON
   NS_THEME_RESIZER:  

then 'focused' is used uninitialised.

I'm wondering if this is fallout from
https://bugzilla.mozilla.org/show_bug.cgi?id=474878, but I'm
far from certain.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2010-01-29 18:48:39 PST
Nah, it looks like the bogus code dates back to at least Firefox 3.0.  See http://mxr.mozilla.org/firefox/source/widget/src/windows/nsNativeThemeWin.cpp which has the same issues.

In fact, it seems to date back to this checkin in CVS:

3.18 <timeless@mozdev.org> 2005-08-20 00:11
Bug 172751 nsITheme support for Windows 9x/NT/2000
patch by tim@prismelite.com r=hyatt sr=roc+moz
Comment 2 Julian Seward [:jseward] 2010-11-22 10:03:55 PST
Created attachment 492353 [details] [diff] [review]
a fix
Comment 3 Jim Mathies [:jimm] 2011-05-24 11:51:40 PDT
Created attachment 534852 [details] [diff] [review]
fix

one line patch to fix an uninitialized variable in theme code.
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-24 13:30:05 PDT
Comment on attachment 534852 [details] [diff] [review]
fix

Review of attachment 534852 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 5 Dão Gottwald [:dao] 2011-05-25 00:32:46 PDT
http://hg.mozilla.org/mozilla-central/rev/4b1200504a4c

Note You need to log in before you can comment on or make changes to this bug.