415037 - "success" returned uninitialized from XPCVariant::VariantDataToJS

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P3)

Description

[Daniel Veditz [:dveditz]]

Fired up my debug build post-FF3b3 crash landings and hit an uninitialized variable "success" referenced in the return statement of XPCVariant::VariantDataToJS

On the stack was nsSAXXMLReader, apparently processing "http://newsrss.bbc.co.uk/rss/newsonline_world_edition/front_page/rss.xml"	(redirected from http://fxfeeds.mozilla.org/rss20.xml).

Specifically it's calling its HandleEndElement, which after a layer of js engine calls ends up in XPCVariant::VariantDataToJS with a T_INTERFACE_IS. In the "last ditch chance to prevent us from double-wrapping" chunk it IsWrappedJS and appears to successfully come up with a JSObject (though doesn't check the return value from GetJSObject--I guess a wrapper can't lose it's guts?). Since it _is_ an object it skips over the "if not an object" chunk that sets success, and thus returns with success uninitialized.

Setting success=PR_TRUE; after getting the object seems to work, but are there other cases in the bit switch that don't return but wouldn't fall into the "if(xpctvar.type.TagPart() == TD_PSTRING_SIZE_IS" bit? If so might be safer to also initialize success up at the top.

Comment 1

[Peter Van der Beken [:peterv]]

(In reply to comment #0)
> though doesn't check the
> return value from GetJSObject--I guess a wrapper can't lose it's guts?

It can, but then the QI just above will probably fail.

Comment 2

[Mike Schroepfer]

So what to  do with this bug?

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: v1

Let's just fix this.

Comment 4

[Peter Van der Beken [:peterv]]

Comment on attachment 301731 [details] [diff] [review]
v1

Trivial fix for an unitialized variable.

Comment 5

[Daniel Veditz [:dveditz]]

This looks OK, but you have to do a lot of work to satisfy yourself that it's OK (check all the types that break out of the switch and verify they'll fall into the various code paths). Unless there's a huge perf bottleneck here it'd be safer to also initialize success to PR_FALSE up at the top, just a little future-proofing.

Comment 6

[Peter Van der Beken [:peterv]]

(In reply to comment #5)
> This looks OK, but you have to do a lot of work to satisfy yourself that it's
> OK (check all the types that break out of the switch and verify they'll fall
> into the various code paths).

I don't think it's that complicated. Once you're out of the switch, there's three possibilities: some strings, wrapped JS with a valid JS object, anything else. All three set success now.