254067 - instanceof extension to handle brutal sharing violates cross-window rules

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Ryan Frishberg]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a3) Gecko/20040726

This deals with how native objects in two browser windows affect each other.  
I'm not exactly sure what the desired behavior is, but asking here.

Reproducible: Always
Steps to Reproduce:
1.  Create an array in a window
2.  Open up a new window, and extend the array object (or not..results are 
same):

Array.prototype.reverseIt = function(){}

3.  Perform the following tests:

alert(window.opener.data.constructor === Array);
alert(window.opener.data.constructor === window.opener.Array);
alert(window.opener.data instanceof Array);
alert(window.opener.data instanceof window.opener.Array);

Actual Results:  
false
true
true
true

Expected Results:  
false
true
false
true

I'm not sure if my expected results are correct b/c it definately, definately 
helps a lot of times to be able to figure out if data from another window is 
an array (using instanceof).  However, shouldn't the constructor 
test/instanceof test be the same?  Also, what about extending the Array object 
in one window?  If you extend it in one window:

alert(window.opener.Array === Array);

returns false.

Comment 1

[Brendan Eich [:brendan]]

> However, shouldn't the constructor test/instanceof test be the same?

Not according to ECMA-262 Edition 3, which does not specify what happens when
there is more than one global object (as happens all the time with the DOM level
0, as you note).

What SpiderMonkey is doing here, in extending instanceof, is intended to support
precompilation using one set of standard constructors, and execution using many
others.  This is an important optimization, but without the engine extension
you've noticed, /hi/ instanceof RegExp would evaluate to false.

This extension works by comparing the internal (native) function pointer shared
by all native class constructors, in the event that instanceof would otherwise
evaluate to false.  If the internal function pointers are equal, instanceof
evaluates to true.

This instanceof extension now seems bogus, though, in light of other work
completed long after it, to ensure that compiler-created instances have their
private data (the expensive part to create, hence the optimization's win of
amortizing that cost over many executions of one precompiled script) wrapped by
cloned objects of the right run-time class.  See bug 165201.

To summarize literal objects and how they are created:

1.  Object literals, e.g. {p:42, q:true, r:'three'}: these are created at
runtime, no precompilation optimization advantage at present (we could optimize
these cases, but do not).

2.  Array literals, e.g. [1, 2, 3]: ditto.

3.  Function "literals", aka function declarations, expressions, and (this last
is an extension to ECMA-262 Edition 3) statements: these are compiled into
objects, but wrapped as needed with clones to share the (expensive to recreate)
private data (the JSFunction and its JSScript).

4.  RegExp literals, e.g. /hi/ or /bye/g (the global flag irrelevant to cloning
issue).  As with functions, these are compiler-created.  Before the patch for
bug 165201, as part of the uber-patch for bug 169559, the compiler-created
object was exposed to different scripts at runtime, even in the case where the
run-time scope differed from the compile time -- in particular the run-time
standard RegExp constructor was different from the compile-time one.

Before the bug 165201 patch landed, we covered up for one problem inherent in
sharing a compiler-created regexp among different run-time executions with the
instanceof extension.

> Also, what about extending the Array object in one window?

I'm not sure what you mean here.  What about it?

If you make an array (or any other object) using a class constructor C in window
A, the instance's prototype will be A.C.prototype, and its constructor property
delegates to A.C.prototype.constructor, which is A.C.  This is necessary for
integrity and composability of scripts loaded among different windows, for
security, and for backward compatibility.

It doesn't matter how you extend an object.  The dependency on the object's
prototype is created when the instance is created, based on the constructor
called (implicitly in the case of [] and {} literals).

Confirming and taking, to remove the instanceof extension and satisfy the
"Expected Results" truth table in comment 0.

/be

Comment 2

[Brendan Eich [:brendan]]

Changing summary to match bug.

/be

Comment 3

[Brendan Eich [:brendan]]

See bug 22048, which was where the instanceof extension to cope with "brutal
sharing" (precompilation followed by N >> 1 executions of the precompiled
script) was done.

I mentioned four classes of compiler-created objects, two of which (object and
array literals) always were given the right scope under brutal-sharing, one
(functions) fixed during the XUL brutal sharing work four years ago, and the
last (regexps) fixed during the 1.8 alpha period on the trunk.

But CVS archeology, and the comment in fun_hasInstance for the instanceof
extension, bring up the Error, SyntaxError, etc. cases.  Some of these are made
at compile time, but thrown then too.  So there's no conflict in scope heritage
at run-time.

However, for some reason, folks hacking on SpiderMonkey in 1999 wanted to make
(e instanceof Error) when really, (e instanceof otherWin.Error), i.e., when
catching an exception thrown by a function scoped by otherWin.  See jsfun.c
revision 3.18, e.g.
(http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=mozilla%2Fjs%2Fsrc&file=jsfun.c&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=1999%2F09%2F21+10%3A00%3A00&maxdate=1999%2F09%2F22+10%3A00%3A00&cvsroot=%2Fcvsroot).

I propose to break this bogus equivalence between Error constructors (likewise,
SyntaxError, EvalError, etc.) among all windows that can call one another's
functions.  I doubt anyone will care, but I'm prepared to learn the hard way
that I'm wrong.  Better now, during 1.8a3, than later.

Patch next.

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: proposed fix

Inspecting the places in the compiler (jsemit.c and jsscan.c) that optimize
based on identity of compile- and run-time scope turned up a bug in
script_compile, which would show up if you precompiled a Script object (new
Script(src) given a string src, or script.compile(src2) to recompile an
existing script with new source in src2) and ran it in different scopes, and
the source used regexp literals and wanted each run of the script to get its
own instances, with the right heritage (so that /hi/ instanceof RegExp at
run-time).

/be

Comment 5

[Ryan Frishberg]

I was a little confused about what the results should be before, but you 
clarified everything.  I'm not sure about all steps you do behind scenes for 
literals, but sounds difficult.  Thanks a lot be for looking at this and 
giving such a great explanation. 

Comment 6

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 155049 [details] [diff] [review]
proposed fix

Excellent, another Error-only edge case cleared up! r=shaver.

Comment 7

[Brendan Eich [:brendan]]

Fixed.  This can't go onto the aviary or 1.7 branches without the big patch for
bug 169559 and bug 165201, and probably all the other trunk changes, also being
merged onto those branches.

/be

Comment 8

[[e-mail]]

In the JS console, [] instanceof top.Array now returns false.
Was this intentional? I don't think the JS console tries to extend Array.
For instance, this seems to have caused bug 256766.

Comment 9

[Brendan Eich [:brendan]]

*** Bug 274122 has been marked as a duplicate of this bug. ***