Closed Bug 415037 Opened 13 years ago Closed 13 years ago

"success" returned uninitialized from XPCVariant::VariantDataToJS

Categories

(Core :: XPConnect, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta4

People

(Reporter: dveditz, Assigned: peterv)

References

Details

Attachments

(1 file)

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.
Flags: blocking1.9?
(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.
So what to  do with this bug?
Attached patch v1Splinter Review
Let's just fix this.
Assignee: jst → peterv
Status: NEW → ASSIGNED
Attachment #301731 - Flags: superreview?(jst)
Attachment #301731 - Flags: review?(jst)
Blocks: 384632
Version: unspecified → Trunk
Attachment #301731 - Flags: superreview?(jst)
Attachment #301731 - Flags: superreview+
Attachment #301731 - Flags: review?(jst)
Attachment #301731 - Flags: review+
Comment on attachment 301731 [details] [diff] [review]
v1

Trivial fix for an unitialized variable.
Attachment #301731 - Flags: approval1.9?
Attachment #301731 - Flags: approval1.9? → approval1.9+
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.
(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.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Priority: -- → P3
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.