129519 - JS_GetPropertyDesc gives up too easily

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Robert Ginda]

JS_GetPropertyDesc totally gives up if access to the property fails, for any
reason.  For example, if the property being looked up is a getter defined in
script, and that getter throws an exception, JS_GetPropertyDesc fails
completely.  Besides the exception thrown, other information about the property
is completely valid and useful.  I propose we add a JSPD_ERROR flag, which is
set on the property description to indicate an exception was thrown while
fetching the property value, and record the exception as the value of the
property in this case.

This is especially useful when JS_GetPropertyDesc is called from
JS_GetPropertyDescArray, where a single failure in JS_GetPropertyDesc results in
a completely empty pd array.

Comment 1

[Robert Ginda]

patch coming up...

Comment 2

[Robert Ginda]

Attachment: proposed patch

"I can see clearly now the rain is gone..."

Comment 3

[Robert Ginda]

ok, so the comment and bracing style is wrong... I've got to run to the garage,
I'll fix it when I return.

Comment 4

[Robert Ginda]

Attachment: patch with formatting cleanup

Comment 5

[Robert Ginda]

Attachment: slightly better patch

Moved pd->flags under else clause to avoid double set.

Comment 6

[Robert Ginda]

Attachment: js/jsd changes

add the new flag to js/jsd files.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 73090 [details] [diff] [review]
slightly better patch

Don't you want to clear cx->throwing after stealing away cx->exception and
succeeding?

/be

Comment 8

[Robert Ginda]

Attachment: even better

save exception state before fetching the property, restore it after.

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 73198 [details] [diff] [review]
even better

Use JS_SaveExceptionState and JS_RestoreExceptionState, for smaller code and
consolidated mechanism?

/be

Comment 10

[Robert Ginda]

I was looking to avoid the malloc/free overhead, but I guess it's not that big
o' deal.  I'll attach a new patch in a bit.

Comment 11

[Brendan Eich [:brendan]]

Actually, the mandatory malloc and free under the JSExceptionState API are evil,
but I wasn't consulted, and the API functions have been there for too long for
me to want to break compatibility (by letting the caller allocate the state
struct on the stack or however, passing it into Save as well as Restore and Drop).

I made a deal with rginda, he's got a fast-path patch coming up.

/be

Comment 12

[Robert Ginda]

Attachment: best ever

Remove unnecessary lastException initialization, and move wasThrowing to the
else clause.

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 73315 [details] [diff] [review]
best ever

do this:

    wasThrowing = cx->throwing;
    if (wasThrowing) {
	JS_AddRoot(cx, &lastException);
	lastException = cx->exception;
	cx->throwing = JS_FALSE;
    }

Later, restore cx->throwing = wasThrowing; and no else clauses needed.

Fix that and r=[e-mail]; I'm too close to this code, shaver should
sr.

/be

Comment 14

[Robert Ginda]

Attachment: all the rest were crap, this is the one

What brendan said, plus combine assign/test for extra bonus points.

Comment 15

[Robert Ginda]

Attachment: brendan says, "Lines are cheap."

split assignments and tests.

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 73335 [details] [diff] [review]
brendan says, "Lines are cheap."

Since we're squishing layers here (not using the malloc-happy JS_Save / Restore
/ DropException APIs), and since the rest of jsdbgapi.c uses the internal
jsgc.h js_AddRoot/js_RemoveRoot calls, don't use JS_AddRoot and JS_RemoveRoot
here.  Do supply a root name such as "lastException" when calling js_AddRoot.

Fix that and r=[e-mail].

/be

Comment 17

[Robert Ginda]

Attachment: js/src changes

Comment 18

[John Bandhauer]

Comment on attachment 73821 [details] [diff] [review]
js/src changes

>+    if (wasThrowing) {
>+        js_AddRoot(cx, &lastException, "lastException");
>+        lastException = cx->exception;
>+        cx->throwing = JS_FALSE;
>+    }

You should set lastException before calling js_AddRoot. lastException
ought not be in an indeterminate state when you add the root.

If we cared about perf then a check here and below to see if it is 
gcthing would be good.

>+    
>+    if (!js_GetProperty(cx, obj, sprop->id, &pd->value)) {
>+        if (!cx->throwing)
>+            return JS_FALSE;

Huh, you are possibly leaving a dangling root here. You can't do that.

Comment 19

[Brendan Eich [:brendan]]

Comment on attachment 73821 [details] [diff] [review]
js/src changes

Curses, schooled by jband.

An uninitialized root is a bad idea, but currently harmless if the request
model is followed.  The GC cannot nest under js_AddRoot and after the root was
added, and it can't run until a later End or YieldRequest.

Early return if (!cx->throwing) will leak the root, but we do want to return
early there.  May as well add a js_RemoveRoot call.

Also, check for js_AddRoot failure and propagate with early return false.

/be

Comment 20

[Robert Ginda]

Attachment: js/src changes, again

Init lastException before rooting, check if lastException is a GC thing before
rooting, unroot before early return, and pass runtime, not cx, to
js_RemoveRoot.

Comment 21

[Mike Shaver (:shaver emeritus)]

Comment on attachment 73873 [details] [diff] [review]
js/src changes, again

>+    if (!js_GetProperty(cx, obj, sprop->id, &pd->value)) {
>+        if (!cx->throwing) {
>+            if (wasThrowing && JSVAL_IS_GCTHING(lastException))
>+                js_RemoveRoot(cx->runtime, &lastException);
>+            return JS_FALSE;
>+        }

I think you want to restore lastException here as well: the getter
could have tried-and-caught (some getters in Mozilla-land do, I
believe) before failing.

Comment 22

[Robert Ginda]

Attachment: js/src changes

what shaver said.

Comment 23

[Robert Ginda]

Attachment: js/src, for the nth time

Why not return useful information for an API failure too?  It simplifies things
a bit and avoids the early return.

I'm not sure how I feel about the JSPD_API_FAILED name, any other suggestions?

Comment 24

[Mike Shaver (:shaver emeritus)]

I think JSPD_ERROR should become JSPD_EXCEPTION, and JSPD_API_FAILED should
become JSPD_ERROR.

Comment 25

[Robert Ginda]

Attachment: js/src, n+1

JSPD_* changes as suggested by shaver.

Comment 26

[Robert Ginda]

Attachment: js/jsd changes, updated to use reflect new defines

Comment 27

[John Bandhauer]

Comment on attachment 73894 [details] [diff] [review]
js/src, n+1

r=jband looks right to me.

Comment 28

[John Bandhauer]

Comment on attachment 73895 [details] [diff] [review]
js/jsd changes, updated to use reflect new defines

r=jband

Comment 29

[Mike Shaver (:shaver emeritus)]

Comment on attachment 73894 [details] [diff] [review]
js/src, n+1

sr=shaver.

Comment 30

[Mike Shaver (:shaver emeritus)]

Comment on attachment 73895 [details] [diff] [review]
js/jsd changes, updated to use reflect new defines

sr=shaver.

Comment 31

[Brendan Eich [:brendan]]

Comment on attachment 73894 [details] [diff] [review]
js/src, n+1

>+        if (JSVAL_IS_GCTHING(lastException))
>+            js_AddRoot(cx, &lastException, "lastException");

I still say you want something like this:

	if (JSVAL_IS_GCTHING(lastException) &&
	    !js_AddRoot(cx, &lastException, "lastException")) {
	    return JS_FALSE;
	}

instead.

>+        cx->throwing = JS_FALSE;
>+    }
>+    
>+    if (!js_GetProperty(cx, obj, sprop->id, &pd->value)) {
>+        if (!cx->throwing) {
>+            pd->flags = JSPD_ERROR;
>+            pd->value = JSVAL_VOID;
>+        } else {
>+            pd->flags = JSPD_EXCEPTION;
>+            pd->value = cx->exception;
>+        }

Cool, good idea -- although it's possible something was reported but not
thrown.  That would be the native getter's fault, and just make for noise.  You
could always temporarily nullify the error reporter, but I don't think it's
worth the trouble

a=[e-mail] if you check for js_AddRoot failure.

/be

Comment 32

[Brendan Eich [:brendan]]

Comment on attachment 73895 [details] [diff] [review]
js/jsd changes, updated to use reflect new defines

>Index: jsdebug.h
>===================================================================
>RCS file: /cvsroot/mozilla/js/jsd/jsdebug.h,v
>retrieving revision 3.14
>diff -u -r3.14 jsdebug.h
>--- jsdebug.h	27 Feb 2002 09:24:12 -0000	3.14
>+++ jsdebug.h	13 Mar 2002 17:07:38 -0000
>@@ -1299,6 +1299,10 @@
> #define JSDPD_ALIAS      0x08    /* property has an alias id */
> #define JSDPD_ARGUMENT   0x10    /* argument to function */
> #define JSDPD_VARIABLE   0x20    /* local variable in function */
>+#define JSDPD_EXCEPTION  0x40    /* exception occurred looking up proprety, */
>+                                 /* value is exception */
>+#define JSDPD_ERROR      0x80    /* native getter returned JS_FALSE without */
>+                                 /* setting an exception */

s/setting/throwing/ in that last comment, and in jsdbgapi.h now that I see this
horrid duplication.

Why can't the jsdbgapi.h defines be used here?

> /* this is not one of the JSPD_ flags in jsdbgapi.h  - careful not to overlap*/
> #define JSDPD_HINTED     0x800   /* found via explicit lookup */
> 
>Index: idl/jsdIDebuggerService.idl
>===================================================================
>RCS file: /cvsroot/mozilla/js/jsd/idl/jsdIDebuggerService.idl,v
>retrieving revision 1.24
>diff -u -r1.24 jsdIDebuggerService.idl
>--- idl/jsdIDebuggerService.idl	27 Feb 2002 09:24:14 -0000	1.24
>+++ idl/jsdIDebuggerService.idl	13 Mar 2002 17:07:42 -0000
>@@ -1151,6 +1151,10 @@
>     const unsigned long FLAG_ARGUMENT  = 0x10;
>     /** local variable in function */
>     const unsigned long FLAG_VARIABLE  = 0x20;
>+    /** exception occurred looking up property, value is exception */
>+    const unsigned long FLAG_EXCEPTION = 0x40;
>+    /** native getter returned JS_FALSE without setting an exception */
>+    const unsigned long FLAG_ERROR     = 0x80;
>     /** found via explicit lookup */
>     const unsigned long FLAG_HINTED    = 0x800;
> 

Wow, triplication.  I see the IDL prob, but not the need for C duplicates.

a=[e-mail] with comment tweak and some reassurance about redundancy
reduction in the future.

/be

Comment 33

[Robert Ginda]

> a=[e-mail] with comment tweak and some reassurance about redundancy
> reduction in the future.

The JSDPD_ dupes have been with the jsd API since inception.  I could mark the
existing consts as deprecated, and not introduce any new ones.  Is that what you
had in mind?

Comment 34

[John Bandhauer]

> The JSDPD_ dupes have been with the jsd API since inception. 

This was based on my perception (what, 4 years ago) that I could not easily
change stuff in the JS engine and that it was good to limit the number of js/src
header files required to build and use jsd. This is one place where ugly
duplication resulted - but which I considered a minor cost at the time. That was
then. It is fine with me if the duplicates go away - or get directly #defined in
terms of the js #defines and not the underlying raw numbers.

> Actually, the mandatory malloc and free under the JSExceptionState API are evil

Me again. I urged mccabe to build a system that included the malloc/free cost of
saving and restoring exception state. At the time there was uncertainty about
other fields that *might* be added to the exception state. And there was a hope
to maintain binary compatibility for embedders that might drop in subsequent
engines without recompiling. So, it seemed like a bad idea to use a caller
supplied buffer. There are ways around that problem, of course. But, as it
happened, we decided to build a simple system where the engine managed that data
(it need not always be implemented to map to the C runtime malloc, BTW). 

FWIW, It would not bother me if we added additional support for caller alloc'd
save and restore functions and deprecated the existing functions. Changing over
the callers in our tree would be trivial.

Comment 35

[Brendan Eich [:brendan]]

I'm just wondering why jsdebug.h couldn't include jsdbgapi.h and rename JSPD_*
to JSDPD_* if it really wanted to.  jsdebug.h alreayd includes jsapi.h.  What's
to be gained by hiding jsdbgapi.h, exactly?

/be

Comment 36

[Brendan Eich [:brendan]]

jband: I'm ok with JS_Save/Restore/DropExceptionState using malloc, I wasn't
gonna bloat the API with caller-allocated versions.  I think we all agreed that
for jsdbgapi.c or other inside-the-engine files, it may pay to inline the fairly
trivial guts of these APIs, and specialize away various costs, esp. malloc/free.

I didn't bother with that in my jsexn.c patch that added a 'stack' property, of
course, because there were lots of costs in that code (the Exception object
constructor, but hey, we don't turn OOM into an exception, so memory should be
available for other errors-as-exceptions :-) already.  I think all is well here,
except (now that we're adding JSPD_ flags) the duplication trade-off.  

I hope I was not hard to deal with in the old days -- I *thought* I catered to
your every need!

/be

Comment 37

[John Bandhauer]

>  JSPD_* to JSDPD_*

All the same to me. But, this may force *someone* using the engine to have to go
change their own code to deal with the loss of one set of symbols or the other.
Whatever.

> What's to be gained by hiding jsdbgapi.h

Now, I don't know. Then it pulled in a lot of headers that did not otherwise
need to be included in order to use jsd. IIRC some of those headers were not
normally even exported to dist by the build system back then. jsd used them
internally, but I wanted to (and did) limit the jsd user's include of js headers
to only be jsapi.h (and the files it includes).

No, Brendan. You were plenty helpful then. But the process of getting in changes
that where not otherwise necessary to the main 4.x product were a bitch. JSD was
a marginal feature. I took that to heart at the time and avoided asking for
permission to makes changes to address problems that I could reasonably work around.

Comment 38

[Brendan Eich [:brendan]]

Confusion: I mean "define JSDPD_* renaming macros in terms of the JSPD_*
counterparts, not by duplicating magic numbers" when I wrote "I'm just wondering
why jsdebug.h couldn't include jsdbgapi.h and rename JSPD_* to JSDPD_* if it
really wanted to."  Sorry, my use of "rename" was misleading -- I meant renaming
macros to preserve jsdebug.h source compatibility (with some pollution from the
nested include of jsdebug.h).

I don't recall jsdbgapi.h including private headers, ever -- js*api.h files try
not to do that.  Just quibbling.

Yeah, 4.x was a bitch -- don't remind me!

/be

Comment 39

[Robert Ginda]

Attachment: js/jsd changes again

While making the JSDPD_ renames JSPD_ changes, I happened upon the meaning of
JSDPD_HINTED (nice name.;)  This flag means that the property doesn't exist on
the object, but *can* be found on the prototype chain.	jsd_val.c constructs a
JSPropertyDesc of it's own in this case, and we need the ERROR/EXCEPTION checks
in there too.

I made the s/setting/throwing/ change and added the js_AddRoot check (do'h,
didn't mean to leave that out earlier) in js/src, I can attach another patch if
anyone wants to see it.

Comment 40

[Brendan Eich [:brendan]]

Comment on attachment 74016 [details] [diff] [review]
js/jsd changes again

Doesn't pd.value need to be rooted?

/be

Comment 41

[Robert Ginda]

> Doesn't pd.value need to be rooted?

I thought so too, at first.  It turns out that _newProperty copies pd.value into
a JSDProperty object, and *that* object holds the root.  Notice that before this
patch pd.value was also not rooted.  Unless GC runs on another thread, we're ok.

Comment 42

[Brendan Eich [:brendan]]

Yeah, it looks safe -- just don't add any JS_New{Object,String,Double}* calls
before the root, or a last-ditch GC could nest on this thread/in this request.

/be

Comment 43

[Robert Ginda]

Comment on attachment 74016 [details] [diff] [review]
js/jsd changes again

jband/ shaver, can I carry your r/sr's over to this patch, or would you like to
stamp this one?

Comment 44

[John Bandhauer]

Comment on attachment 74016 [details] [diff] [review]
js/jsd changes again

r=jband

Comment 45

[Robert Ginda]

Comment on attachment 74016 [details] [diff] [review]
js/jsd changes again

shaver says to move his sr forward

Comment 46

[Robert Ginda]

checked in

Comment 47

[Phil Schwartau]

Marking Verified -