571823 - UMR [@ nsHTMLSelectOptionAccessible::GetStateInternal]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Jesse Ruderman]

Attachment: testcase (requires accessibility service to be enabled)

1. Run Firefox under Valgrind.

2. Enable accessibility, e.g. by pasting the following into the js console
   Components.classes["@mozilla.org/accessibilityService;1"]
     .getService(Components.interfaces.nsIAccessibleRetrieval);

3. Load the testcase

Result: Valgrind complains of a UMR [@ nsHTMLSelectOptionAccessible::GetStateInternal]

Looks like GetStateInternal expects nsHTMLSelectOptionAccessible::GetSelectState to always set its out-params, and that isn't happening in this case.

Comment 1

[Jesse Ruderman]

Attachment: details from valgrind (--num-callers=50 --track-origins=yes)

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: reflex WIP

Comment 3

[Jesse Ruderman]

Comment on attachment 451106 [details] [diff] [review]
reflex WIP

This patch fixes the Valgrind warning for me :)

Comment 4

[alexander :surkov (:asurkov)]

Please initialize local variables passed into GetSelectState.

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch

This patch makes it clear that we expect these args to be init to 0. Also fixed a typo while I'm here.

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 451286 [details] [diff] [review]
patch


> nsIFrame* nsHTMLSelectOptionAccessible::GetBoundsFrame()
> {
>-  PRUint32 state;
>+  PRUint32 state = 0;
>   nsCOMPtr<nsIContent> content = GetSelectState(&state);
> 
> nsIContent* nsHTMLSelectOptionAccessible::GetSelectState(PRUint32* aState,
>                                                          PRUint32* aExtraState)
> {
>+  *aState = *aExtraState = 0;

this is a crash

Comment 7

[Boris Zbarsky [:bzbarsky]]

> this is a crash

Why?

Comment 8

[Jesse Ruderman]

It didn't crash for me when I tried it last night.

Comment 9

[alexander :surkov (:asurkov)]

because aExtraState is null in the case of GetBoundsFrame().

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> It didn't crash for me when I tried it last night.

probably you didn't trigger this code.

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch 2

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

http://hg.mozilla.org/mozilla-central/rev/b24c59785359
(Thanks guys)