Closed
Bug 415037
Opened 17 years ago
Closed 17 years ago
"success" returned uninitialized from XPCVariant::VariantDataToJS
Categories
(Core :: XPConnect, defect, P3)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla1.9beta4
People
(Reporter: dveditz, Assigned: peterv)
References
Details
Attachments
(1 file)
742 bytes,
patch
|
jst
:
review+
jst
:
superreview+
mtschrep
:
approval1.9+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Comment 1•17 years ago
|
||
(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•17 years ago
|
||
So what to do with this bug?
Assignee | ||
Comment 3•17 years ago
|
||
Let's just fix this.
Assignee: jst → peterv
Status: NEW → ASSIGNED
Attachment #301731 -
Flags: superreview?(jst)
Attachment #301731 -
Flags: review?(jst)
Updated•17 years ago
|
Version: unspecified → Trunk
Updated•17 years ago
|
Attachment #301731 -
Flags: superreview?(jst)
Attachment #301731 -
Flags: superreview+
Attachment #301731 -
Flags: review?(jst)
Attachment #301731 -
Flags: review+
Assignee | ||
Comment 4•17 years ago
|
||
Comment on attachment 301731 [details] [diff] [review]
v1
Trivial fix for an unitialized variable.
Attachment #301731 -
Flags: approval1.9?
Updated•17 years ago
|
Attachment #301731 -
Flags: approval1.9? → approval1.9+
Reporter | ||
Comment 5•17 years ago
|
||
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.
Assignee | ||
Comment 6•17 years ago
|
||
(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: 17 years ago
Priority: -- → P3
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•