336040 - Bad scope chain for objects break eval in subscript loader with custom scope

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[James Ross]

Ok, you'll have to bear with me on this one - this is a really hard bug to describe. I do know it is a regression from bug 304376, though.

I've managed to get the scope chain of a few objects at various points, which might help explain what's going on as it works its way towards failure.

1. Loading the XUL window. For each <script/>, we get:
  A. nsJSContext::ExecuteScript - aScopeObject
       Object     Scope      Class
    #  Addr       Addr       Addr     Flags    Name
    00 059e0d60   058bdeb8   047114c0 0001000d ChromeWindow

  B. nsJSContext::ExecuteScript - mContext->globalObject
       Object     Scope      Class
    #  Addr       Addr       Addr     Flags    Name
    00 058ffe58   059a6450   047114c0 0001000d ChromeWindow

This is the first interesting thing; the scope being passed to the JS engine to execute the JS files included via <script/> is not the same as the global object for the context. This doesn't seem important to me, but...

2. Doing a normal eval("stuff") in one of the <script/>s.
  A. obj_eval - scopeobj
       Object     Scope      Class
    #  Addr       Addr       Addr     Flags    Name
    00 068ce868   06bcc520   0064b060 05020005 Call
    01 068cef28   06811538   0064b060 05020005 Call
    02 059e0d60   058bdeb8   047114c0 0001000d ChromeWindow

Note that the scope chain matches the aScopeObject passed from ExecuteScript.

3. Doing a subscript load.
  A. mozJSSubScriptLoader::LoadSubScript - target_obj
       Object     Scope      Class
    #  Addr       Addr       Addr     Flags    Name
    00 06918dc0   0705d838   0064b03c 01000000 Object
    01 058ffe58   059a6450   047114c0 0001000d ChromeWindow

Here's the start of the real problems: the object created in the JS loaded via <script/> - and then passed to the subscript loader - has the "other" scope at the end. That results in...

4. Doing eval("stuff") inside the subscript that was loaded.
  A. obj_eval - scopeobj
       Object     Scope      Class
    #  Addr       Addr       Addr     Flags    Name
    00 0695e0a8   06c20ad0   0064b060 05020005 Call
    01 06918cd0   06804d48   0064b03c 01000000 Object
    02 06918dc0   0705d838   0064b03c 01000000 Object
    03 058ffe58   059a6450   047114c0 0001000d ChromeWindow

And this then b0rks out because the ChromeWindow is JSCLASS_EXTENDED, and returns 0x058bdeb8 for the innerObject. The result is the wonderfully incorrect exception (though quite deliberate):
  EvalError: function eval must be called directly, and not by way of a
  function of another name

The nsJSContext::ExecuteScript chains were printed at:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/dom/src/base/nsJSEnvironment.cpp&rev=1.279&mark=1202#1194

The mozJSSubScriptLoader::LoadSubScript chains were printed at:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/xpconnect/loader/mozJSSubScriptLoader.cpp&rev=1.20&mark=207#197

The obj_eval chains were printed at:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsobj.c&rev=3.257&mark=1222#1220

So far, the only testcase is to load a script that uses eval into ChatZilla. I'll see if I can get a simpler testcase and/or put a script online. What I know for certain is that the eval exception does not occur prior to bug 304376 being checked in, which I've double-checked by doing build-by-date.

Comment 1

[James Ross]

Just to make it a bit clearer, the code using the subscript loader creates a |new Object| first and passes that as the scope object to it. It's that new object that has the "wrong" scope chain.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

A couple of answers:

(In reply to comment #0)
> This is the first interesting thing; the scope being passed to the JS engine to
> execute the JS files included via <script/> is not the same as the global
> object for the context. This doesn't seem important to me, but...

This is expected. The global object passed into the JS engine in the inner window and cx->globalObject is the outer window.

> Here's the start of the real problems: the object created in the JS loaded via
> <script/> - and then passed to the subscript loader - has the "other" scope at
> the end. That results in...

Ouch, this is bad. Parentage should *always* lead to the inner window, not the outer window, which seems to have happened here and is the cause of the bug (since calling eval with an object parented to the outer window must throw a security error to avoid security problems (though I agree that error message needs to be tweaked for the new world)).

Comment 3

[James Ross]

Thanks for clearing up which bit is not right. I think I'll do some poking in js_InvokeConstructor, but I think any problem occurs at a higher level.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Hey James, could you give more precise steps to reproduce? I just attempted to /load a script that did:
var obj = new Object();
eval("", obj);

and didn't see any errors. I also loaded a script which reportedly hits this bug and didn't see anything wrong either. Did this get automagically fixed by one of the follow-up checkins?

Comment 5

[James Ross]

Blake, it's not eval you pass the scope object to, it's the subscript loader.

The ChatZilla plugin that was originally hitting this bug doesn't appear to do so on current trunk any more, although there's now no less than 5 scripts failing to load due to *something* else (and also reporting really screwed error objects).

I asked for a dependency chain to be correct set in at least one of the regression bugs from bug 304376 that might be related to scope chains, but since that's clearly not happened I've got no clue if any relevant bugs have been fixed.

You're welcome to guess what bug fixed it, although the current behaviour is significantly more broken than before. :(

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #5)
> The ChatZilla plugin that was originally hitting this bug doesn't appear to do
> so on current trunk any more, although there's now no less than 5 scripts
> failing to load due to *something* else (and also reporting really screwed
> error objects).

This sounds like bug 340893, which will be fixed by bug 340019. The problems caused by that bug got a lot worse when brendan checked in the additional generator (send, close, etc.) stuff since unreported and uncaught pending exceptions now prevent us from entering the interpreter at all, meaning that we won't execute any scripts until the exception is handled somehow.

Comment 7

[James Ross]

Much as I hate this bug, it appears to be in trunk still. I can't be sure it went away, either, as I've not used a trunk build since comment 5 (at which point far too much broke to use it). When I get time, I'll play with my debugger.