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

Status: RESOLVED FIXED

(Core Graveyard :: Installer: XPInstall Engine, defect)

Description

[Johnny Stenback (:jst)]

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.

Comment 1

[Johnny Stenback (:jst)]

Attachment: Use INT_TO_JSVAL() when storing ints in jsvals.

Comment 2

[Johnny Stenback (:jst)]

Attachment: Use INT_TO_JSVAL() when storing ints in jsvals.

Ahem, this is the version I was trying to attach here.

Comment 3

[Daniel Veditz [: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.

Comment 4

[Johnny Stenback (:jst)]

Attachment: So maybe like this then?

Comment 5

[Daniel Veditz [:dveditz]]

Comment on attachment 162222 [details] [diff] [review]
So maybe like this then?

yup, that'll do it. r/sr=dveditz

Comment 6

[Johnny Stenback (:jst)]

Fixed.

Comment 7

[Daniel Veditz [:dveditz]]

Probably want this on the aviary/1.7 branches, right?

Comment 8

[Johnny Stenback (:jst)]

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.