Closed Bug 64111 Opened 24 years ago Closed 24 years ago

XPConnect vs. Object.prototype.toSource woes

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
Windows NT
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: rcashman, Assigned: jband_mozilla)

Details

Attachments

(8 files)

proposed fix: make Components non-enumerable in window; return true from GetAttributes
1.29 KB, patch
Details | Diff | Splinter Review
add toSource and toString to wrapped natives
6.21 KB, patch
Details | Diff | Splinter Review
fix silent failure and failure to indicate count of zero in enumerating wrapped natives
1.36 KB, patch
Details | Diff | Splinter Review
proposed fix ready to go!
29.08 KB, patch
Details | Diff | Splinter Review
checkin comment to accompany patch above
2.86 KB, text/plain
Details
a proposed fix that addresses brendan's concerns
27.69 KB, patch
Details | Diff | Splinter Review
slightly better patch - eats lookup errors
27.56 KB, patch
Details | Diff | Splinter Review
new patch reverting to old Object.prototype caching scheme
30.36 KB, patch
Details | Diff | Splinter Review 
From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 5.5; Windows NT 4.0; LEXIS®)
BuildID:    Mozilla/5.0 (Windows; U; WinNT4.0; en-US; m18) Gecko/20001108 
Netscape6/6.0 

When variables are set using language="JavaScript1.2" the variables are 
unavailable, even immediately after they are set.  JavaScript1.1 and 
JavaScript1.3 do not exhibit this behavior.

Reproducible: Always
Steps to Reproduce:
1.Select make test window
2.Return to original browser
3.Select dump test window
Use HTML in additional info

Actual Results:  After step 1 a new browser was spawned
When going back to the original browser and selecting dump test window, nothing 
happened

Expected Results:  In step one a new browser should have been spawned (it was) 
and an alert box should have come up with the following: 
  After open win: [object window].  
We did not get the alert.
After step 3 we should get an alert box with the following:
  win [object window]
This part failed.


<html><head><script language="JavaScript1.2"> 
var win=""; 
function makewind() { 
win = open("about:blank", "_blank"); 
alert("After open win: " + win);
} 
function dumpwind() { 
alert("win "+win+"\n"); 
} 
</script></head><body> 
<button type=button onclick="makewind()">make test window</button><br> 
<button type=button onclick="dumpwind()">dump test window</button><br> 
</body></html>
This is a possible duplicate of bug 57534.

Is the problem that the alerts came up but the value of win was undefined?  If 
so, this is definitely a duplicate of bug 57534.
This could be a duplicate of bug_57534.  It would interest me to know why we 
did not see the same behavior for JavaScript1.1 or JavaScript1.3.
also, be sure to use a new nightly - 0.6 is old.
Bug 57534 was marked fixed, and I'm seeing this problem on a build I made from 
source checked out this morning. It seems that there's a problem converting the 
window object to a string. For instance, if you change the dumpwind() function 
above to this:

function dumpwind() {
  alert("A);
  var a=win;
  alert("B");
  var b=6;
  alert("C "+b);
  alert("D "+win.closed);
  win.toString(); // or equivalently a="win is "+win
  alert("E");
}

then you hit the first four alerts (through "D false"), but the next alert never 
fires; I'm pretty sure it exits the function while trying to do the 
win.toString(). All this only if you've specified JavaScript1.2. Anything else 
(including no language spec at all) and it works fine.

This smells like an issue we have every now and then where some object in the C 
code is returning the wrong kind of error or not returning an error -- excuse me 
while my hands wave around I'm working from a fuzzy memory here -- and the JS 
interpreter is somehow acting correctly by exiting the function early. I know 
Brendan knows what I'm talking about, if I've described the situation at all 
correctly, so I'm adding that unfortunate to the cc: list (and confirming the 
bug).
Status: UNCONFIRMED → NEW
Ever confirmed: true
I poked around a bit here and from stepping through the JS engine code it looks
like this is a bug in js_obj_toSource() (which doesn't get called in this case
if the JS version is other than 1.2), there's code in that method that does:

    he = js_EnterSharpObject(cx, obj, &ida, &chars);
    if (!he)
        return JS_FALSE;

and with this testcase js_EnterSharpObject() returns null and we return JS_FALSE
and that causes the script execution to fail w/o an error message. Since no DOM
code is executed here to my knowledge, I think this is a JS Engine bug, Brendan,
wanna look into this, or can you reassign to whomever this belongs to?
Over to jband.

http://lxr.mozilla.org/mozilla/source/js/src/xpconnect/src/xpcwrappednativejsops.cpp#524

looks like it should be a ThrowException call instead, before line 525 which
does return JS_FALSE, creating a silent error.

/be
Assignee: jst → jband
Help me out here Brendan...

I agree my WrappedNative_GetAttributes is wrong. 

BTW, I cloned this from liveconnect:
http://lxr.mozilla.org/seamonkey/source/js/src/liveconnect/jsj_JavaObject.c#752

I'm not so sure that the right solution is to throw an exception. We could 
either a) throw and return false or b) not throw and return true. I tried both 
and neither just fixed this problem.

If it throws then the script stops and we at least see an error - but the script 
still stops and really shouldn't - should it?.

If it doesn't throw and returns true then I still don't see the original test 
work right... For the 'makewind()' test I see the new window but no alert box. 
But there is an error:

JavaScript error:
chrome://communicator/content/securityUI.js line 31: Components.classes['@mozill
a.org/secure_browser_ui;1'] has no properties

I don't know about that. I just pulled and built and maybe some PSM thing is 
screwed up?

But when I do the dumpwind() test I still don't get an alert. But this time I 
get an "Access to XPConnect service denied." exception with a stack ending like:

nsScriptSecurityManager::CheckXPCCapability(JSContext * 0x038d93e0, const char *   
0x00000000) line 1336
nsScriptSecurityManager::CanGetProperty(nsScriptSecurityManager * const   
0x027b8c14, JSContext * 0x038d93e0, const nsID & {...}, nsISupports *   
0x04f06910, nsIInterfaceInfo * 0x00aa9a60, unsigned short 0x0005, const long   
0x0331f230) line 1459 + 25 bytes
nsXPCWrappedNativeClass::CallWrappedMethod(JSContext * 0x038d93e0,   
nsXPCWrappedNative * 0x04f043d0, const XPCNativeMemberDescriptor * 0x03fb5174, 
nsXPCWrappedNativeClass::CallMode CALL_GETTER, unsigned int 0x00000000, long *   
0x00000000, long * 0x0012d988) line 609 + 63 bytes
nsXPCWrappedNativeClass::GetAttributeAsJSVal(JSContext * 0x038d93e0,   
nsXPCWrappedNative * 0x04f043d0, const XPCNativeMemberDescriptor * 0x03fb5174,   
long * 0x0012d988) line 936
WrappedNative_GetProperty(JSContext * 0x038d93e0, JSObject * 0x014342a8, long   
0x0331f230, long * 0x0012d988) line 272 + 24 bytes
MarkSharpObjects(JSContext * 0x038d93e0, JSObject * 0x014342a8, JSIdArray * *   
0x00000000) line 377 + 27 bytes
MarkSharpObjects(JSContext * 0x038d93e0, JSObject * 0x014337a0, JSIdArray * *   
0x0012da10) line 388 + 34 bytes
js_EnterSharpObject(JSContext * 0x038d93e0, JSObject * 0x014337a0, JSIdArray * *   
0x0012da94, unsigned short * * 0x0012da80) line 438 + 17 bytes
js_obj_toSource(JSContext * 0x038d93e0, JSObject * 0x014337a0, unsigned int   
0x00000000, long * 0x01471ec8, long * 0x0012dbf4) line 565 + 21 bytes
js_obj_toString(JSContext * 0x038d93e0, JSObject * 0x014337a0, unsigned int   
0x00000000, long * 0x01471ec8, long * 0x0012dbf4) line 822 + 25 bytes

The JS engine is trying to get a property of an nsIBrowserInstance that the 
running JS code would not be allowed to access. This throws and our script fails 
and no alert runs.

What is supposed to go on here? Is there a safe way to stop js_obj_toSource from 
digging too deep into the xpconnect wrapped object? It is crapping out on the 
first xpidl 'attribute' in the interface; i.e. xpconnect will let it get the 
function objects for the members (but not call them w/o permission), but it 
won't allow fetching the value of attributes (really calling a getter on the C++ 
object).
Assignee: jband → brendan
That LiveConnect JavaObject_getAttributes code is wrong, it should return true.
Beard is fixing it in the next patch for bug 59686.

Even with that fix and the XPConnect counterpart of it, we'll fail to be
backward compatible as you note, because we've hung enumerable properties off of
window that throw exceptions when you try to get their values.  If we can't make
those properties (Components?) non-enumerable, we need to suppress errors under
js_obj_toSource.  Comments?

/be
Jband, sorry to bounce this bug but it has your name all over it ;-) -- if
anyone on the cc: list can r=, you can sr= it as far as I'm concerned.

/be
Assignee: brendan → jband
Fixing summary.  One reason to make a property non-enumerable in JS is to avoid
for..in loop crawlers from traipsing over "bad stuff".  A better reason is the
ad-hoc solution to this backward compatibility bug, but both reasons together
make a strong case, IMO.  Comments?

/be
Summary: Variables unavailable when using JavaScript1.2 → Components object should be non-enumerable
brendan,

Renaming the bug doesn't fix it :)

The original testcase does not trip over enumerating 'Components'. It trips over  
an nsIBrowserInstance hanging off of the window. Your patch does not make the 
testcase work.

I actually think Components *ought* to be enumerable as a property of window 
when present.

It seems to me that the right fix is to have wrapped natives implement toSource 
in the way that they implement toString... calling it on the wrapped object if 
it is found in the interface, else just implementing it to spit out a string and 
not traverse the members. I'm inclined to do this. What do you think?
I didn't mark it fixed, at least!  Sorry for the brain-fart, and tell me if the
latest summary isn't better.  Also, the xpcwrappednativejsops.cpp half of my
patch is good, right?

Yeah, I think you do have to handle toSource a la toString.  There won't be more
such methods in the future, ECMA and I promise.

My misplaced point questioning whether "bad/insecure stuff below" or "internal"
properties should be enumerable still applies, in a sense: instead of blaming
enumerability of window.Components, or of the global variable that refers to
that nsIBrowserInstance (which class must die, but never mind!), let's blame
poor old XPConnect for not providing a "censoring" toSource.

/be
Summary: Components object should be non-enumerable → XPConnect vs. Object.prototype.toSource woes
Brendan, I got code working to expose toSource and toString on wrapped natives 
(FWIW, I was doing toString functionality in the Convert handler, but not 
actually exposing a toString method), but my toSource is not being called as I 
had expected. MarkSharpObjects just nests into the properties of properties 
without checking to see if a given property is an object with its own toSource 
method. It seems to me that it ought to check for a toSource method on each 
property that is an object and use it if found rather than just digging right 
into that object's properties on its own. What do you think?

Also here is a a simple xpcshell testcase (note that the toSource called by 
Object.prototype.toString fails when 'o' has a wrapped native property):

js> version(120)
0
js> var o = {a:1, b:2}
js> o
{a:1, b:2}
js> o.c = Components
[xpconnect wrapped nsIXPCComponents]
js> o
js> o.c = 3
3
js> o
{a:1, b:2, c:3}
js>

I'll attach my patch to add toSource to wrapped natives (though the testcase 
reamains unfixed).
Well, really, my little xpcshell testcase above shows a different problem than 
the original testcase. In the original testcase we are tripping over the 
security restriction on getting a property. In my xpcshell testcase it is 
silently failing even without any security problems.

I'll attach a patch that deals with the silent failure in nsIXPCScriptable code 
and also fixes bad logic in wrapped native enumeration when zero properties are 
found.

However, I'm still seeing odd stuff with:

  var o =  {c : Components};
  version(120);
  print(o);

I see stack overflows in either MarkSharpObjects or js_MarkGCThing. If I use...
  var o =  {c : Components.interfaces};
...then all is well and it prints...
  {c:xpconnect wrapped nsIXPCComponents_Interfaces}
...(though I'm not sure why it is finding my toSource in this case only)

But if I use... 
  var o =  {c : Components};
...or...
  var o =  {c : Components.classes};
...then I see the stack overflows either while doing thr sharp stuff or while 
gc'ing.

This is all with my toSource/toString patches in place.

Two more problems to fix:

1) The use of nsXPCWrappedNativeClass::NewInstanceJSObject was not walking 
up the parent chain to use an appropriate parent when making new wrapper 
JSObjects. So, in some cases, we were ending up with some extremely deep parent 
chains in the JSObjects of wrapped natives. I have a fix for that.

2) nsIJSID has an 'id' member that when wrapped becomes another (distinct) 
instance of nsIJSID. This gives an infinite regression. The id member makes 
sense for C++ callers so that they can get the actual nsid, but for JS callers 
it doesn't do much good anyway. I think the thing to do is make the id member 
[noscript].
An update...

I have this working but its not done yet.

I'm past my confusion about how Object.prototype.toSource works. Just 
implementing toSource on xpconnect wrapped natives was not enough because 
MarkSharpObjects was still traversing the properties of the wrappers and trying 
to get property values that the security manager would not allow.

I fixed this by modifying the wrapped native enumeration scheme. Now when 
enumerating the 'static' properties of a wrapped native (those methods, 
attributes, and consts declared in the xpidl interface) xpconnect only exposes 
in the enumeration loop those properties that the caller could do a 'Get' on. 
All the consts and methods can be 'got' (security checking on methods is done at 
method call time), but for the attributes xpconnect checks with the security 
manager to verify whether or not the given attribute can be read. If the 
security manager vetos then xpconnect does not include the property in the 
enumeration. This seems to work well enough.

I'm torn on what exactly should happen in a toSource call on an xpconnect 
wrapped native. I discussed this with mccabe. Suppose that we have...

  var o = {c : Components};
  dump(o.toSource());

With the patch above you'd get:
 {c:xpconnect wrapped nsIXPCComponents}
This produces invalid object literal syntax. Other options are:
 {c:"xpconnect wrapped nsIXPCComponents"}
and
 {c:{}}

In the long run with the conversion of the DOM to use xpconnect (I think) we 
want toSource to actually show the properties of wrapped natives like any other 
object. This implies that the wrapped native should delegate toSource calls to 
Object.prototype.toSource

Until now the JSObject of wrapped natives has had a null prototype. This is 
actually sort of bogus. I have a patch to get the appropriate Object.prototype 
upon creation of each xpcwrappednativescope and to use it as the proto of each 
wrapper in that scope [in the coming flattened world those wrappers around 
objects with a known (and shared) classinfo will get a shared proto in the given 
scope. Object.prototype will be that shared proto object's proto].

Anyway, I've been playing with letting these wrapped natives use the 'real' 
toSource. The problem with that is that the output for the tree of the 
Components object (including the interfaces and classes arrays) is voluminous!

But, in general I think this makes sense.

I'm thinking to add a flag that xpconnect gets via nsIXPCScriptable::GetFlags to 
indicate that the given object wishes to *not* inherit Object.prototype.toSource 
and instead will use a toSource in xpconnect that yields only '{}'. Components 
(and its properties) will set this flag and will not result in huge toSource 
output.

Still, when doing Object.prototype.toSource on any JSObject that has Components 
as a property, MarkSharpObjects will run over these JSObjects and force an eager 
resolve of a lot of objects that otherwise might not need to be created. I'm not 
seeing a clear way to avoid this. Perhaps brendan is right in saying that when 
xpconnect adds Components to the global object it should not set it as 
enumerable.

Thoughts?

I'll pull this together and get a unified patch together.

Common Lisp has had big fun with such deep object to source conversions, and
even JS had unintended ones in the 4.x timeframe (the last expression statement
in a script evaluated to a window reference, and until we fixed it, the native
script evaluation code would convert the result -- in the eval sense -- into a
string, which could be quite enormous).

I think Components should be unenumerable for maximum backware compatibility and
safety.  Independent of that, if someone calls toSource on it, it can produce
something that will not round-trip through eval (don't forget to use uneval
rather than toSource for proper primitive type handling): if you delegate to
Object.prototype.toSource, eval(uneval(Components)) won't be a valid Components
object hierarchy, it'll be a bunch of vanilla JS Objects.

The toSource method and uneval are dead ends, as far as ECMA is concerned.  To
"finish" them is hard to impossible, requiring domain-specific syntax for
serializing eval'able native class and private data information.  If every
native class had a scriptable constructor that took arguments (or a template
Object) specifying properties and private data, maybe.

But the problem isn't worth solving, so I vote for {} as the result of the
default XPConnect wrapper toSource, if there isn't a toSource method defined in
the "wrapee".

/be

    
    

    
  
+    if(sz)
+    {
+        JSString* str = JS_NewString(cx, sz, strlen(sz));
+        if(str)
+        {
+            *vp = STRING_TO_JSVAL(str);
+            return JS_TRUE;
+        }
+    }
+    JS_ReportOutOfMemory(cx);
+    return JS_FALSE;

Oops, this will double-report OOM if JS_NewString fails, then leak sz.

If you handle toString and toSource ids in WrappedNative_GetProperty but not in
WrappedNative_LookupProperty, will 'with (xpc) alert(toString())' fail?

I'm wondering why we don't just lookup Object.prototype (as the new operator
does) when making a new wrapper instance.  That would allow for weird but maybe
wonderful substitutions of Object.prototype to cohere with wrapper prototypes,
and it would match JS semantics.  It would also remove the per-scope (is that
per-window?) rooted prototype-object pointer.

More in a bit, thought I'd dash these off fast.

/be


    
    

    
  
how about...

    if(!sz)
    {
        JS_ReportOutOfMemory(cx);
        return JS_FALSE;
    }

    JSString* str = JS_NewString(cx, sz, strlen(sz));
    if(!str)
    {
        JS_smprintf_free(sz);
        // JS_ReportOutOfMemory already reported by failed JS_NewString
        return JS_FALSE;
    }
    
    *vp = STRING_TO_JSVAL(str);
    return JS_TRUE;

JSOP_NEW cares about what you are creating. I'm just trying to get a vanilla 
Object.prototype (which we've lived without 'till now). I'm trying to save the 
cost of looking it up dynamically each time we create a wrapper. I'm not sure I 
understand what would be gained by doing so. Who would change it and why? When 
flattening is working then instances of a class will get their own shared proto. 
But wrappers without classinfo will still work like this. Maybe you could say 
more about why this should be different?

I've been making WrappedNative_LookupProperty stay in sync with GetProperty as 
good form. I'm not really sure how and where it would make a difference. The 
fact that xpconnect is not doing the right thing with JSProps may make this 
somewhat moot right now. In the new world it appears that interface stuff will 
have to hang off the proto (in actual slots) anyway to make dynamic DOM property 
lookup work right.

Thanks.

    
    

    
  
So I had a look at the patch and not knowing a whole lot about XPConnect
internals I can only say that the changes look good to me (apart from what
brendan found). I found one nit tho:

+    if(NS_OK != sm->CanGetProperty(cx, GetIID(), wrapper->GetNative(),
+                                   GetInterfaceInfo(),
+                                   desc->index, desc->id))

Why not use if (NS_SUCCEEDED(sm->...)?

r=jst

    
    

    
  
Thanks Johnny. I originally spec'd nsIXPCSecurityManager to specifically use 
NS_OK to mean "no veto". I can't remember why I thought it mattered at the time. 
This was before we decided that all non-NS_OK success codes were evil. I've been 
consistently using this 'NS_OK !=' pattern in my use the interface. I should fix 
them all in one shot and improve the comment in the interface header to be more 
precise. For now I'd rather be consistent.

http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/idl/nsIXPCSecurityManag
er.idl#67


    
    

    
  
jband: looks good on that string code.  The reason to look up Object.prototype
on each new instance is so that user changes to the value of Object.prototype
change the __proto__ of subsequent new instances, just as they do in JS for
subsequent 'var o = new Object' or 'var o = {hi:"there"}', e.g.

If you don't return some kind of non-null property pointer from LookupProperty
for toSource and toString, then the with statement example I gave will fail, and
any use of an XPConnect wrapper on the scope chain (say, as a global object)
will also result in failures to find those identifiers by their unqualified (no
foo.toString, only toString) references.

/be

    
    

    
  
OK. I can change the Object.prototype lookup stuff.

Also note that I *do* do LookupProperty stuff the same as GetProperty stuff for 
toSource and toString.

I'll attach a patch with the Object.prototype lookup change later today.

Thanks.

    
    

    
  
All good, but thinking about eval(uneval(Components)) more, I wonder if we
should not just throw an exception, rather than uneval to "({})".  Why lie when
we can tell the truth?

/be

    
    

    
  
window has plenty of properties that happen to be wrapped natives. If we throw 
an exception when trying to run toSource on them then we end up back with this 
bug broken. No?

    
    

    
  
Duh, of course -- all those vars referring to nsIBrowserInstance instances, e.g.

I'll consider a better "I can't provide eval'able source" representation in a
separate bug, if necessary.  sr=brendan@mozilla.org on your latest patch.

/be

    
    

    
  
As much as I'd like to check this in, the dynamic lookup of 'Object.prototype'
uncovered a deeper problem...

That 'Object.prototype' lookup sometimes fails. It fails in a 'callback' from
C++ into a wrapped JS object when the window's JSContext is not on the context
stack. IN that case, xpconnect uses the default JSContext for the given (in this
case, main) thread. However, that JSContext is from the hidden window and does
not have the same codebase as the window in which the called JS code actaully
came from. So, the security check in lookups for properties of the global
(window) object fails with an exception. The implications here really really
suck. I *think* this might mean that the called JS code wouldn't even be able to
access global vars even when the vars are declared in the vary same html file as
the running code.

Do we have this problem generically outside of xpconnect? e.g... if window 'A'
has var 'v' and function 'f'. If 'f' is passed to window 'B' and then some event
handler causes 'B' to call 'f' on 'B's JSContext, then can 'f' NOT access its
own 'v'? Or am I just missing it?

The caps code compares the calling JSContext with the JSContext of the window
(whose property is being accessed).  But, does it get down to looking at the
codebase of the *function* that is trying to access the property? If so, then I
guess the problem is not so generic and it is a problem for me because I'm
trying to access the property via the JSAPI and not from a compiled function in
the language. Otherwise, this seem like a bigger problem.

I could use a clue from someone better informed. Else, I'll dig in more and see
what I can find.

    
    

    
  
OK, no insight from anyone on the protential security issue I pointed out above.

For the purposes of this bug I can set that issue aside (since it really does
not matter now if the Object.prototype fail). Except, that if there is a lookup
failure I need to have xpconnect eat the error report and exception. I'll attach
a patch that includes a new class in xpconnect - AutoJSErrorAndExceptionEater -
that will eat errros and exceptions while in scope and then clean itself up as
it goes out of scope. I added the use of this in the Object.prototype lookup and
in one other place where I was doing this manually.

I'm looking for a r= and sr= on this minor variation of the previously revied
code. Thanks..



    
    

    
  
jband wrote:
>Do we have this problem generically outside of xpconnect? e.g... if window 'A'
>has var 'v' and function 'f'. If 'f' is passed to window 'B' and then some
>event handler causes 'B' to call 'f' on 'B's JSContext, then can 'f' NOT access
>its own 'v'?

No, JS uses static scoping.  f's scope chain depends only on the 'variables
object' in the execution context used when f's declaration was evaluated (ECMA
jargon; in SpiderMonkey, what was fp->varobj when the JSOP_DEFFUN bytecode was
interpreted).  That object is window A, and if A.v exists, references to v in
f's body will resolve to A.v, not to B.v or undefined.

So, it seems caps (or caps + xpconnect) is using dynamic scope for security
checks.  That is, it uses cx to reach something (a global object) that owns a
principal.  This is wrong.  It should use cx->fp->script to get the principal.

It turns out that ECMA requires Object.prototype, etc. to be permanent|readonly,
so there is no chance that users can reset such properties for system classes.
(I was confused because ECMA allows f.prototype for a user-defined function f to
be reset).

jband, I'm sorry to push you down a rat-hole.  Yur prototype-rooting code is
better; it matches ECMA's recurrent "original value of Foo.prototype" spec,
which seems to say that new functions, dates, arrays, etc. all delegate not to
the present value of Function.prototype, etc., but to the "original" value.

So is patch http://bugzilla.mozilla.org/showattachment.cgi?attach_id=22265
("proposed fix ready to go!") good enough, or does it need a tweak or two from
the subsequent mostly-or-all-rathole patches?

/be


    
    

    
  
Just to finish that thought: the JS engine does indeed do a runtime lookup of
"Object" in the last object on the scope chain, and then "prototype" in the
result of the first lookup, to find the proto of the new instance being created
(e.g., for Object; same for Date, etc.).  I do that because there is no good
place to store roots for the standard class prototypes, and the object graph is
strong, reference-wise.

But I'm trading time to save space, because roots in some kind of per-top-level
or "last in scope chain" object could be contrived, with enough work.

Note also that while Object.prototype is permanent and readonly, Object is not,
and users may override Object in the top-level scope.  That's another reason
favoring dynamic binding.  But ECMA specifies "the original value of
Object.prototype" (etc.) in several places.  Argh.  Anyway, I don't think your
rooted-Object.prototype-snapshot code will surprise any uses, in practice.

/be

    
    

    
  
OK. I'm fine with going back to looking up Object.prototype on a per-scope basis 
like before. That older patch is no longer best though. I want to keep some 
of the newer stuff and add some changes:

- use the new AutoJSErrorAndExceptionEater.
- use  the precalc'd jsids of Object and prototype and use OBJ_GET_PROPERTY (as 
  I use elsewhere).
- add code to unroot my reference to Object.prototype in 
  nsXPCWrappedNativeScope::SystemIsBeingShutDown() to allow better cleanup and
  avoid the leaked root warnings.

I'll attach the patch.

r= and sr= please

Status: NEW → ASSIGNED

    
    

    
  
jband: is there a bug on file about the XXX comment in xpcwrappedjsclass.cpp,
the one pondering a provisional exception to compare when the script is done and
it did terminate abnormally?

One too many spaces, it's too spacey:

+                *statep =  INT_TO_JSVAL(index+1);

Whooops, ToStringGuts reverted to the leaky version from an old patch:

+    if(sz)
+    {
+        JSString* str = JS_NewString(cx, sz, strlen(sz));
+        if(str)
+        {
+            *vp = STRING_TO_JSVAL(str);
+            return JS_TRUE;
+        }
+    }
+    JS_ReportOutOfMemory(cx);
+    return JS_FALSE;

Wahhh, you switching to two-space indentation in mid-stream?

+    // Lookup 'globalObject.Object.prototype' for our wrapper's proto
+    {
+      AutoJSErrorAndExceptionEater eater(cx); // scoped error eater

/be


    
    

    
  
> One too many spaces, it's too spacey:

fixed.

> Whooops, ToStringGuts reverted to the leaky version from an old patch

Thanks! I lost this one in the patching and didn't notice in my once-over.

> Wahhh, you switching to two-space indentation in mid-stream?

I've been doing this in these blocks that exist only to control the lifetime of 
an autoclass (locking, JS request, etc.) I *imagine* that it sets them off and 
makes the reader notice. This is donce consistently in this corner of Rome.

Do you want to see the nit fixes or trust me? I'm inclined to consider r=jst as 
still applicable and go on your sr=

    
    

    
  
> jband: is there a bug on file about the XXX comment in xpcwrappedjsclass.cpp,

filed bug 66453 and added a link to that bug to the comment in the code.


    
    

    
  
r=jst is still applicable and I even had one more look at the patch and I don't
see any problems.


    
    

    
  
The ToStringGuts leak is not merely a nit, but I trust ya -- sr=brendan@mozilla.org.

/be

    
    

    
  
Fix checked in.

brendan: know the StringGuts bit wasn't a nit - and I'm glad you found it - but 
fixing it was just a matter of copy and paste of the reviewed function body.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED

    
    

    
  
Verified with 2001-02-06-08-Mtrunk
Status: RESOLVED → VERIFIED

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: