nsInstall stores ints in jsval's w/o using INT_TO_JSVAL()

RESOLVED FIXED

Status

Core Graveyard
Installer: XPInstall Engine
RESOLVED FIXED
14 years ago
3 years ago

People

(Reporter: jst, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

14 years ago
nsInstall::GetWinProfile() and a few other methods store an int in a jsval
directly w/o making sure it's flagged as an int, and shifted properly etc. The
code needs to use INT_TO_JSVAL(). This makes errors from those methods come out
to the installer JS code with the wrong value. Patch coming up.
(Reporter)

Comment 1

14 years ago
Created attachment 162214 [details] [diff] [review]
Use INT_TO_JSVAL() when storing ints in jsvals.
(Reporter)

Updated

14 years ago
Attachment #162214 - Flags: superreview?(dveditz)
Attachment #162214 - Flags: review?(dveditz)
(Reporter)

Comment 2

14 years ago
Created attachment 162216 [details] [diff] [review]
Use INT_TO_JSVAL() when storing ints in jsvals.

Ahem, this is the version I was trying to attach here.
Attachment #162214 - Attachment is obsolete: true
(Reporter)

Updated

14 years ago
Attachment #162216 - Flags: superreview?(dveditz)
Attachment #162216 - Flags: review?(dveditz)
(Reporter)

Updated

14 years ago
Attachment #162214 - Flags: superreview?(dveditz)
Attachment #162214 - Flags: review?(dveditz)
Comment on attachment 162216 [details] [diff] [review]
Use INT_TO_JSVAL() when storing ints in jsvals.

>     *aReturn = JSVAL_NULL;
> 
>     PRInt32 result = SanityCheck();
> 
>     if (result != nsInstall::SUCCESS)
>     {
>-        *aReturn = SaveError( result );
>+        *aReturn = INT_TO_JSVAL(SaveError( result ));
>         return NS_OK;
>     }

This is just wrong, these functions return an *object* and that JSVAL_NULL
is the error case (scripts can check for it). Only functions that return
a status code should be trying to save that error. These three routines should
do

    *aReturn = JSVAL_NULL;
    if (SanityCheck() != nsInstall::SUCCESS)
	 return NS_OK;

A function that sometimes returns a usable object and sometimes a numeric error
code is bad news, makes scripting overly complicated.

I don't like the javscript polution in these functions, it would have been
cleaner separation to return the appropriate install object and keep the js
object-wrapping code in the javascript glue layer (nsJSInstall.cpp). Don't need
to fix that now.
Attachment #162216 - Flags: superreview?(dveditz)
Attachment #162216 - Flags: superreview-
Attachment #162216 - Flags: review?(dveditz)
Attachment #162216 - Flags: review-
(Reporter)

Comment 4

14 years ago
Created attachment 162222 [details] [diff] [review]
So maybe like this then?
(Reporter)

Updated

14 years ago
Attachment #162222 - Flags: superreview?(dveditz)
Attachment #162222 - Flags: review?(dveditz)
Comment on attachment 162222 [details] [diff] [review]
So maybe like this then?

yup, that'll do it. r/sr=dveditz
Attachment #162222 - Flags: superreview?(dveditz)
Attachment #162222 - Flags: superreview+
Attachment #162222 - Flags: review?(dveditz)
Attachment #162222 - Flags: review+
Attachment #162216 - Attachment is obsolete: true
(Reporter)

Comment 6

14 years ago
Fixed.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Probably want this on the aviary/1.7 branches, right?
(Reporter)

Comment 8

14 years ago
Either way is fine with me, doesn't seem like a lot of people are running into
this, so I can deal w/o it on the branches.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.