22048 - XUL brutal sharing needs super-prototype love

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Chris McAfee]

Got a JS emoticon error this morning,
input text field stopped accepting input and
I had to quit the app.  Not sure how I did this,
if you need more info I can research the problem more.

Comment 1

[Robert Ginda]

(changing component to chatzilla)

Was the message something like "emoticon.search has no properties" around line
193 in test-static.js?

afaik, that's the only place in the code it's actually referred to as an
"emoticon".  This patch *should* prevent the hard error, but I've got no idea
how it actually occurs.  I'll check this in when the tree opens.

Index: munger.js
===================================================================
RCS file: /cvsroot/mozilla/extensions/irc/xul/lib/munger.js,v
retrieving revision 1.1
diff -u -r1.1 munger.js
--- munger.js   1999/11/29 03:57:33     1.1
+++ munger.js   1999/12/17 21:38:10
@@ -64,7 +64,7 @@
         else
             ary = text.match(this.entries[entry].regex);

-        if (ary != null)
+        if ((ary != null) && (ary[1]))
         {
             var startPos = text.indexOf(ary[1]);

Comment 2

[Chris McAfee]

yes, the error looked like that.
Didn't know about chatzilla component, nice!

Comment 3

[Robert Ginda]

this seems to be cause by the sharing bug secribed in bug 22098

*** This bug has been marked as a duplicate of 22098 ***

Comment 4

[Brendan Eich [:brendan]]

THis

Comment 5

[Brendan Eich [:brendan]]

Er, that is to say: this is a distinct bug from 22098 -- see my comment with two
alternative fixes in bug 22098 for more.

/be

Comment 6

[Robert Ginda]

The real patch to make chatzilla work is:

Index: test3-static.js
===================================================================
RCS file: /cvsroot/mozilla/extensions/irc/xul/tests/test3-static.js,v
retrieving revision 1.11
@@ -131,7 +132,8 @@
                                "event-tracer", true /* negate */,
                                false /* disable */);

-    obj.munger = new CMunger();
+    obj.munger = new CMunger();
+    /*
     obj.munger.addRule ("you-talking-to-me?", matchMyNick, "");
     obj.munger.addRule
         ("link", /((http|mailto|ftp)\:\/\/[^\)\s]*|www\.\S+\.\S[^\)\s]*)/,
@@ -140,13 +142,14 @@
         ("face",

/((^|\s)[\<\>]?[\;\=\:\8]\~?[\-\^\v]?[\)\|\(pP\<\>oO0\[\]\/\\](\s|$))/,
          insertSmiley);
-    obj.munger.addRule ("rheet", /(rhe+t\!*)/i, "rheet");
+    obj.munger.addRule ("rheet", /(rhee+t\!*)/i, "rheet");
     obj.munger.addRule ("bold", /(\*.*\*)/, "bold");
     obj.munger.addRule ("italic", /[^sS](\/.*\/)/, "italic");
     obj.munger.addRule ("teletype", /(\|.*\|)/, "teletype");
     obj.munger.addRule ("underline", /(\_.*\_)/, "underline");
     //obj.munger.addRule ("strikethrough", /(\-.*\-)/, "strikethrough");
     obj.munger.addRule ("smallcap", /(\#.*\#)/, "smallcap");
+    */

 }

Comment 7

[Brendan Eich [:brendan]]

I'm going with my solution #1: Call JS_InitStandardClasses(cx) in the XUL code
that makes this "prototype global" object against which XUL scripts are
compiled."  Auto-cloning RegExp objects created by the JS compiler for literal
regular expressions imposes too much overhead on compile/exec evaluation (where
there is no brutal compile early, exec later in multiple contexts, sharing).

Patch coming up.

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: proposed fix (also for 22098)

Comment 9

[Robert Ginda]

Patch fixes both bugs.

Comment 10

[Brendan Eich [:brendan]]

Fixed in rdf/content/src/nsXULPrototypeDocument.cpp.

/be

Comment 11

[Robert Ginda]

This is still showing up in the WinNT builds.
/foo/ instanceof RegExp fails in windows, works as expected in linux debug and
opt.

Comment 12

[Paul MacQuiddy]

not sure who is JS qa, trying prashant

Comment 13

[leger]

Clearing FIXED resolution due to reopen.  Setting QA Contact to rginda

Comment 14

[Brendan Eich [:brendan]]

Not an M13 stopper.  The problem is no longer that /hi/.test is not defined -- we 

fixed that.  Now the problem is that /hi/ instanceof RegExp is false in XUL 

windows because brutal sharing uses a secret global object to scope the 

compilation of /hi/, and then at runtime uses a DOM XUL window to scope the right 

operand, RegExp.  We've broken prototype identity, yay!



This can be fixed in M14, either by sharing one set of standard classes among all 

XUL windows, or by two-layer prototype hierarchy.



/be

Comment 15

[Robert Ginda]

Attachment: Linux degub build test results

Comment 16

[Robert Ginda]

uuh, ignore that attachment.  Wrong bug.

Comment 17

[Brendan Eich [:brendan]]

Argh, two-layer prototype hierarchy is impossible: say you have a XUL window, x, 
and the prototype document d.  Then if you make

d.Object.prototype === x.Object.prototype.__proto__

you can't arrange for

(new x.RegExp("hi")) instanceof x.Object

because JS supports only a single prototype per object 
(x.RegExp.prototype.__proto__ === x.RegExp.prototype, and 
x.RegExp.prototype.__proto__ === x.Object.prototype, and 
x.Object.prototype.__proto__ === null, but there cannot be any 
x.Object.prototype along that chain).

So we're left with one set of standard classes for all XUL windows.  This is 
certainly faster, too.  It requires changing the DOM code in nsGlobalWindow.cpp 
so that ::JS_InitStandardClasses is not always called.  For XUL windows, I think 
we want to clone each standard class constructor function; non-constructors such 
as Math we can share by reference.  We need constructor clones so that instances 
get parented by their particular XUL windows, not by the hidden XUL prototype 
document script object used for compilation.  Hope this is clear enough!

I'll hack up a patch to extend the JS API with JS_CloneStandardClasses.

/be

Comment 18

[Chris Waterson]

Remember, all XUL cometh not from the same security domain. We can load XUL
files just like regular content. So this may require some cleverness in the code
that is init-ing standard classes to determine whether it can share or not.

Comment 19

[Chris Waterson]

moving to M15 because i am doomed.

Comment 20

[Brendan Eich [:brendan]]

I'd go further: XUL loaded from untrusted domains (not from the unified, 
singular domain of chrome:) should not brutally share the prototype document's 
script object for compile-time constructors (Function, RegExp).  Can we have a 
per-origin compile-time scope object?  That would seem to preserve the identity 
relations we crave, without compromising security.

/be

Comment 21

[Brendan Eich [:brendan]]

I'm a dumbass, typos messed up my example (note d for x everywhere after the 
first ==='s left operand):

(x.RegExp.prototype.__proto__ === d.RegExp.prototype, and 
d.RegExp.prototype.__proto__ === d.Object.prototype, and 
d.Object.prototype.__proto__ === null, but there cannot be any 
x.Object.prototype along that chain).

Anyway, same punchline: no "MI" or multiple prototype delegation a la Self in 
JS, wahhh.

/be

Comment 22

[Brendan Eich [:brendan]]

I promised a JS_CloneStandardObjects patch, but thinking about it more, I bet we 
can do even better, in terms of per-XUL-window set-new-document cost.

Instead of cloning a bunch of constructors, some of which may never be called, 
we should specialize the XUL window resolve hook (JSClass.resolve, or perhaps 
better: nsIJSScriptObject::Resolve) to look for undefined properties named by 
XUL scripts in the prototype document's script object.  If a property of the 
same name is found there, check its type (JS_TypeOfValue).  If JSTYPE_FUNCTION, 
then JS_CloneFunctionObject and JS_DefineProperty; else just JS_DefineProperty 
to share the value.

Waterson, I may cough up a patch for this if it'll help you.  I probably won't 
do the work in dom/src/base to avoid calling ::JS_InitStandardClasses, or the 
work to make a per-trust-domain locus of compilation scope and standard classes.

/be

Comment 23

[Brendan Eich [:brendan]]

OK, I don't think this is a js1.5 bug any more (although I still question the 
special case whereby e instanceof Exception is true no matter which 
Exception.prototype e delegates to, in a world of multiple top-level objects and 
their standard classes (including Exception)).

/be

Comment 24

[Chris Waterson]

brendan: backatcha.

Comment 25

[Brendan Eich [:brendan]]

I'm actually interested in revamping the context-universal e instanceof 
SyntaxError (e.g.) behavior, generalizing it so that instanceof works for brutal 
sharing embeddings of JS too.  Also, the way jsfun.c and jsexn.c conspire to 
implement this (js_exnHasInstance) seems flimsy, or hokey, or something -- but I 
haven't spent the time to think of a general way.  I'll do that now.

/be

Comment 26

[Brendan Eich [:brendan]]

Attachment: proposed fix (diff -u format)

Comment 27

[Brendan Eich [:brendan]]

Attachment: proposed fix (diff -wu format)

Comment 28

[Brendan Eich [:brendan]]

Read the -wu diffs for best code-buddying results.  Note also that I'm checking
in the fix for http://bugzilla.mozilla.org/show_bug.cgi?id=33391 (included in
jsfun.c).

/be

Comment 29

[Brendan Eich [:brendan]]

Looking for some r= love tonight (M15 deadline).

/be

Comment 30

[Robert Ginda]

Attachment: minimal xul testcase

Comment 31

[Robert Ginda]

js engine passes all tests, mozilla passes testcase.

Comment 32

[Brendan Eich [:brendan]]

Adding jband.

/be

Comment 33

[John Bandhauer]

r=jband

Comment 34

[Brendan Eich [:brendan]]

Thanks to jband's review, I fixed the #undef WRAP_EXCEPTION to be #undef
MAKE_EXCEPTION_CTOR, and I squished a warning by making the sentinel initializer
at the end of the exceptions table be {0,0,NULL} rather than {0,0}.

/be

Comment 35

[Brendan Eich [:brendan]]

Fixed, thanks to jband, mccabe, and rginda.

/be

Comment 36

[Mike McCabe]

This patch is an improvement for native objects, but I'm not sure it's as
general as it could be for user-defined functions.

With a little dredging, I remembered the original problem with using the
'constructor' property to implement.  (Apologies to Brendan, if this is what you
showed me on John's whiteboard.)  This objection applies to user-generated
functions, might apply to brutal-shared functions, and doesn't apply to
builtins.

The constructor property is a property of the initial, default .prototype
property of newly generated user functions.  In general, user-generated function
hierarchies are built by assinging to function .prototype properties.  This
overwrites the .constructor information, so it can't be used to test whether an
object was built by a given function.


Function A() {}
// A gets .prototype property, and .prototype.constructor === A

Function B() {}
// ditto

// Make A instances get behavior from B.prototype
A.prototype = new B // overwrites A.prototype.constructor
a = new A(); // a.__proto__ === B.prototype
a.__proto__.constructor === B // !

a instanceof A
a instanceof B

In the single-window case, these expressions are both true, by the
prototype-chasing hasInstance.  Is the first expression true in the
brutal-sharing case?  Does the question even come up, for brutally-shared
user-defined functions?

Comment 37

[Brendan Eich [:brendan]]

Good point, the first expression (a instanceof A) is false in the brutal sharing 
case, unless you add A.prototype.constructor = A; after A.prototype = new B.

The (a.__proto__.constructor === B) invariant that you commented with // ! has 
always been an anomaly in this kind of JS, brutal sharing or no brutal sharing. 
I mean, we just constructed a via (new A), so you'd think a's constructor was A, 
eh? [sorry]  OTOH, A.prototype was set to a (new B), and all instances of B 

I remember this coming up in the ancient days, 3.0 or so, and I advised folks 
then to set A.prototype.constructor = A after setting A.prototype = new B.  This 
is exactly what the JS engine does (js_SetClassPrototype) for natives: when you 
set constructor.prototype, you set .prototype.constructor to point back.  So, is 
this policy good enough (if documented -- help me, rginda!) to close the bug?

/be

Comment 38

[Brendan Eich [:brendan]]

Oops, didn't finish editing that last sentence of paragraph 2: "OTOH, 
A.prototype was set to a (new B), and all instances of B " should have read 
"OTOH, A.prototype was set to an instance of B (new B), whose constructor is 
naturally enough B."

Notice that any brutal sharer may set A.prototype.constructor = A, even though 
each sharer gets its own function-object clone for A.  Brutal sharing means that 
either all the function objects (clone-parent when brutally-shared source was 
loaded; clones for each sharing "context") have the same native function 
pointer, or they all have the same shared JSScript.  Brutal sharing thus reduces 
JS-compiled scripts to equivalence in number and shared-ness with C or C++ 
compiled native functions.

/be