Closed Bug 398609 Opened 13 years ago Closed 12 years ago

Simpler handling of hidden properties

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

VERIFIED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

Attachments

(2 files, 18 obsolete files)

v14
151.01 KB, patch
brendan
: review+
brendan
: approval1.9+
Details | Diff | Splinter Review
2.01 KB, patch
Details | Diff | Splinter Review
To implemet the mapping from function argument and variable names into the corresponding slot indexes SpiderMonkey uses hidden properties stored in the function object. 

Although the support for hidden properties is very specific to the function object, it is implemented through generic API, js_LookupHiddenProperty and js_AddHiddenProperty. It leads to a lot of duplicated code to setup arguments to the API and analyze the results at the each call site.

It would be nice to replace the generic API with ones that are tailored to the needs of the code that checks/adds argument and variable names.
Attached patch v1 (obsolete) — Splinter Review
This is a backup of a patch that passes the test suite for the shell. The browser tests come next.
Attached patch v2 (obsolete) — Splinter Review
The patch replaces js_AddHiddenProperty/js_LookupHiddenProperty with js_AddLocal/js_LookupLocal. The new functions take JSFunction * as a parameter and always add/lookup hidden properties to/in JSFunction.object. That allowed to remove the lookup code from fun_resolve.

In few places the patch simplified the code logic based on the fact that the hidden properties are only added to the function object. As such they can not exist in the Call or other scope objects so the patch searches for argument and variable names only when the scope object has js_FunctionClass.

Another cleanup is replacement of pn_attr via boolean pn_const since the attributes were only used to check to distinguish var from const.

Compared with v1 the new version removes too strict assert in FunctionDef from jsparse.c. In the browser the variable object for the function may not have Call class.

The patch passes the tests for jsshell and smoke/bloat tests in the browser.
Attachment #283605 - Attachment is obsolete: true
Attachment #283618 - Flags: review?(brendan)
Will look when I can -- want a compiled code size win, did you get one?

/be
(In reply to comment #3)

For the optimized build of jsshell compiled with GCC 4.1.2 under Linux I got code size reduction by 1120 bytes from 574064 down to 572944.
Attached patch v3 (obsolete) — Splinter Review
The new version simplifies logic in BindNameToSlot from jsemit.c
Attachment #283618 - Attachment is obsolete: true
Attachment #283743 - Flags: review?(brendan)
Attachment #283618 - Flags: review?(brendan)
Blocks: 398808
Attached patch v4 (obsolete) — Splinter Review
The new version makes the argument and variable name lookup in the decompiler to be O(1). For that the patch builds the array that maps argument and variable indexes into the names only once when preparing for function decompilation. 

This is not the final patch as I would like to share that code with XDR support for functions as the code in both places build the same array.

BTW, the patch makes apparent that using hidden properties to implement name-to-index mapping requires rather small amount of code. Thus using something else to implement that mapping would be justified only if it really addresses the bug 398219.
Attachment #283743 - Attachment is obsolete: true
Attachment #283743 - Flags: review?(brendan)
Attached patch v5 (obsolete) — Splinter Review
Changes with v4:

1. Both fun_xdrObject from jsfun.c and JS_NEW_PRINTER calls js_GetLocalNames to get array that maps argument and variable names into indexes to avoid duplicating scope walking code.

2. fun_xdrObject uses a more compact serialization format for argument and variable names with a separated bitmap to denote const variables and parameters without names corresponding to destructive patternes.

3. js_AddLocal is responsible for updating the fun->nargs and fun->u.i.nvars counters and reporting errors on overflow. This reduced the code size for an optimized build of jsshell farther down 571472 from 574064 saving 2592 bytes.

4. js_AddLocal/js_LookupLookal/js_GetLocalNames insist that the passed function is marked as interpreted. For that it was necessary to pass JSFUN_INTERPRETED to js_NewFunction.
Attachment #283922 - Attachment is obsolete: true
Attachment #284224 - Flags: review?(brendan)
Attached patch v6 (obsolete) — Splinter Review
The new version uses null as a setter for hidden properties eliminating js_SetArgument and js_SetLocalVariable functions.
Attachment #284224 - Attachment is obsolete: true
Attachment #284232 - Flags: review?(brendan)
Attachment #284224 - Flags: review?(brendan)
Attached patch v7 (obsolete) — Splinter Review
The new version inlines js_LookupPropertywithFlags in js_LookupName to make js_LookupName infallible.
Attachment #284232 - Attachment is obsolete: true
Attachment #284357 - Flags: review?(brendan)
Attachment #284232 - Flags: review?(brendan)
(In reply to comment #9)
> Created an attachment (id=284357) [details]
> v7
> 
> The new version inlines js_LookupPropertywithFlags in js_LookupName to make
> js_LookupName infallible. 

In addition the patch eliminates js_GetArgument and js_GetLocalVariable and uses instead ((JSPropertyOp) sizeof(jsuword)), ((JSPropertyOp) 2 * sizeof(jsuword)) as a function pointer tags to distinguish hidden properties for arguments and variables.

With the last patch the existence of hidden is pretty much localized to jsfun.c.
Attached patch v8 (obsolete) — Splinter Review
The new version eliminates the dup argument from js_AddLocal and always passes SPROP_IS_DUPLICATE to js_AddNativeProperty when adding hidden names. This works since js_AddNativeProperty searches for an existing property in any case so it can set/clear SPROP_IS_DUPLICATE on its own. With this change js_AddLocal always adds a new property delegating to the caller the job to decide whether to accept duplicated locals or not. Note that such change comes from a patch I am working on to replace the hiddens with an alternative storage and makes that patch smaller.
Attachment #284357 - Attachment is obsolete: true
Attachment #284532 - Flags: review?(brendan)
Attachment #284357 - Flags: review?(brendan)
Blocks: 399544
Attached patch v8b (obsolete) — Splinter Review
This is the previous version syncronized with CVS trunk changes.
Attachment #284532 - Attachment is obsolete: true
Attachment #285536 - Flags: review?(brendan)
Attachment #284532 - Flags: review?(brendan)
Comment on attachment 285536 [details] [diff] [review]
v8b

No need for _TAG suffix on JS_HIDDEN_*_[GS]ETTER names, these are not tags (low or otherwise few bits encoding type in a word containing a pointer or scalar in the other bits), they are pseudo-getter and -setter pointers.

JSLK_ prefix might be more readable as JSLOCAL_ -- the suffixes are short (NONE,ARG,VAR,CONST).

if (JSLK_NONE != js_LookupLocal(...)) in jsemit.c is not house style and does not match js_LookupLocal calls in conditions elsewhere in the patch.

s/localkind/localKind/ in

    JSLocalKind localkind;

in BindNameToSlot. Also:

         * to stack slot. Look for an argument or variable property in the

could wrap better by losing "property", since that's going away in the next bug.

s/can not/cannot/ in

         * We can not optimize the name access when compiling with an eval or

I saw at least one pn->pn_op, should be PN_OP(pn) for C++ purity -- same for any pn->pn_type.

It seems BindNameToSlot could before this patch, and still can after it, change a pn for arguments outside a function from a JSOP_NAME to a JSOP_ARGUMENTS node. Same for the TCF_FUN_USES_NONLOCALS flag-setting. True? Avoid if not compiling in function scope?

Nits about parentfun and localkind -- prevailing style generally camelCaps such names. s/localkind/localKind/ throughout patch.

                parentfun = (JSFunction *)
                            OBJ_GET_PRIVATE(cx,
                                            OBJ_GET_PARENT(cx, fun->object));

and other (JSFunction *) OBJ_GET_PRIVATE(...) expressions cry out for a GET_FUNCTION_PRIVATE macro.

Pre-existing pattern:

                JS_ASSERT(LOCKED_OBJ_GET_CLASS(stmt->down->u.blockObj) ==

in the compiler should probably use STOBJ_... -- more precise, shorter.

In fun_resolve:

     * This is not just an optimization since we must quit the resolve hook
     * when changing hidden properties during the compilation. The setup code
     * for the prototype and the lazy properties below eventually calls the
     * property hooks for the function object. That in turn calls

suggest comma after "optimization", s/since/because/, s/quit the resolve hook/not resolve/, s/changing/defining/, s/the compilation/compilation/.

bitmap stuff in fun_xdrObject should use jsbitmap and the macros from jsbit.h, relieving need for JS_BITS_PER_UINT32.

In js_AddLocal, s/Destructive parameter does not have name/Destructuring parameters have no name/ in

         * Destructive parameter does not have name so we just update the

The comment ending:

     * SPROP_IS_DUPLICATE flag, and not mapped by an entry in scope. The flag
     * is cleared when js_AddNativeProperty finds that the property is unique.

suggests that the flag should be called SPROP_ALLOW_DUPLICATE or some such.

In js_LookupLocal:

         * Inline js_LookupPropertyWithFlags to avoid useless prototype chain

remove "useless" since it's more than that in the technical sense that unwanted effects and reads (uses) could result. Comment should wrap more nicely to boot.

More jsbitmap/jsbit.h opportunity in js_GetLocalNames (worth doing for uniformity and 64-bit jsbitmap on 64 bit CPUs).

More s/destructive/destructuring/, grep -i for destructive and fix.

In jsfun.h, s/of/or/ in

 * Look up an argument of variable name returning its kind when found or

s/on the return/on successful return/ in

 * When bitmap is not null, on the return it will contain a bit array where

and s/variable is a constant/var is really a const/ in

 * is set when the corresponding variable is a constant.

In js_DecompileFunction, once past the #ifdef'ed

        JS_ARENA_RELEASE(&jp->sprinter.context->tempPool, mark);

why not return false on earliest opportunity rather than testing if (ok) {...} and overindenting ... ?

I'll re-review a patch with these nits picked and stamp promptly. Thanks,

/be
(In reply to comment #13)
> bitmap stuff in fun_xdrObject should use jsbitmap and the macros from jsbit.h,
> relieving need for JS_BITS_PER_UINT32.

Currently xdr files are platform-independent. Using jsbitmap would make them platform dependent unless an extra code is added to recode jsbitmap as a sequence of uint32 words. So I will keep uint32-based bitmap code in js_GetLocalNames as fun_xdrObject is the single user of the bitmap support.
Attached patch v9 (obsolete) — Splinter Review
The new version implements the suggestions from the comment 13 except the following:

> I saw at least one pn->pn_op, should be PN_OP(pn) for C++ purity -- same for any pn->pn_type.

The patch does not add new pn->pn_op. It patches the code using either ->pn_op or PN_OP() depending on the existing usage.

> It seems BindNameToSlot could before this patch, and still can after it, change a pn for arguments outside a function from a JSOP_NAME to a JSOP_ARGUMENTS node. Same for the TCF_FUN_USES_NONLOCALS flag-setting. True? Avoid if not compiling in function scope?

I will check this and update the patch as necessary.

> More jsbitmap/jsbit.h opportunity in js_GetLocalNames (worth doing for uniformity and 64-bit jsbitmap on 64 bit CPUs).

I use uint32 to ensure that fun_xdrObject, the single user of the bitmap option of js_GetLocalNames, generates word-size independent encoding.
Attachment #285536 - Attachment is obsolete: true
Attachment #285536 - Flags: review?(brendan)
Attached patch v9b (obsolete) — Splinter Review
the previous patch is too zealous about using GET_FUNCTION_PRIVATE and replaces few calls to (JSFunction *) JS_GetPrivate with the macro where the private slot may not be yet initialized. The new version addresses just that so the patch passes the test suite. The comment 15 is still applies to it.
Attachment #286439 - Attachment is obsolete: true
Attached patch diff between v8b and v9b (obsolete) — Splinter Review
As bugzilla is not up to the job here is a diff between v8b and v9b to show the renames and OBJ_GET_PRIVATE -> GET_FUNCTION_PRIVATE replacements.
The previous diff is v9b itself, not the interdiff.
Attachment #286448 - Attachment is obsolete: true
(In reply to comment #13)
> It seems BindNameToSlot could before this patch, and still can after it, change
> a pn for arguments outside a function from a JSOP_NAME to a JSOP_ARGUMENTS
> node. Same for the TCF_FUN_USES_NONLOCALS flag-setting. True? Avoid if not
> compiling in function scope?

This can not happen neither before nor after the patch. Before the patch the code have:

    obj = fp->varobj;
    clasp = OBJ_GET_CLASS(cx, obj);
    if (clasp != &js_FunctionClass && clasp != &js_CallClass) {
        /* Check for an eval or debugger frame. */
        if (fp->flags & JSFRAME_SPECIAL)
            return JS_TRUE;

        /*
         * Optimize global variable accesses if there are at least 100 uses
         * in unambiguous contexts, or failing that, if least half of all the
         * uses of global vars/consts/functions are in loops.
         */
        optimizeGlobals = (tc->globalUses >= 100 ||
                           (tc->loopyGlobalUses &&
                            tc->loopyGlobalUses >= tc->globalUses / 2));
        if (!optimizeGlobals)
            return JS_TRUE;
    } else {
        optimizeGlobals = JS_FALSE;
    }

...

    if (optimizeGlobals) {
        /*
         * We are optimizing global variables, and there is no pre-existing
         * global property named atom.  If atom was declared via const or var,
         * optimize pn to access fp->vars using the appropriate JOF_QVAR op.
         */
        ATOM_LIST_SEARCH(ale, &tc->decls, atom);
        if (!ale) {
            /* Use precedes declaration, or name is never declared. */
            return JS_TRUE;
        }

For non-functional case clasp != &js_FunctionClass && clasp != &js_CallClass is true so the would be a return before that arguments checking code.

The same holds after the patch although that check now comes in the form of 
if (clasp == &js_FunctionClass) {
...
} else if (clasp != &js_CallClass) {
   the same global checks
}

For clarity I can add an assert to BindNameToSlot before 

    /*
     * We couldn't optimize pn, so it's not a global or local slot name.
     * Now we must check for the predefined arguments variable.  It may be
     * overridden by assignment, in which case the function is heavyweight
     * and the interpreter will look up 'arguments' in the function's call
     * object.
     */
    if (pn->pn_op == JSOP_NAME &&
        atom == cx->runtime->atomState.argumentsAtom) {
        pn->pn_op = JSOP_ARGUMENTS;
        return JS_TRUE;
    }
    tc->flags |= TCF_FUN_USES_NONLOCALS;

to state that from that point the emitter either deal with function or an eval inside the function. I.e something like this:

jS_ASSERT((tc->flags & TCF_IN_FUNCTION) ||
          (fp->fun && (fp->flags & (JSFRAME_EVAL | JSFRAME_COMPILE_N_GO))));
    JS_ASSERT(clasp == &js_FunctionClass);
...
} else if (!fp->fun || !(fp->flags & (JSFRAME_EVAL | JSFRAME_COMPILE_N_GO))) {


But then perhaps it is better to replace:

if (clasp == &js_FunctionClass) {
...
} else if (clasp != &js_CallClass) {
...
} 

by something like:

if (tc->flags & TCF_IN_FUNCTION) {
    JS_ASSERT(clasp == &js_FunctionClass);
...
} else if (!fp->fun || !(fp->flags & (JSFRAME_EVAL | JSFRAME_COMPILE_N_GO))) {
    JS_ASSERT(clasp != &js_CallClass);
...
}

At this point it starts to sound as another cleanup bug...
Attached patch v10 (obsolete) — Splinter Review
The new version changes BindNameToSlot so it contains:

if (clasp != &js_FunctionClass && clasp != &js_CallClass) {
...
    return JS_TRUE;
}

In this way there is no doubts that we inside the function when doing arguments checks.
Attachment #286447 - Attachment is obsolete: true
Attachment #286449 - Attachment is obsolete: true
Attachment #286452 - Flags: review?(brendan)
Comment on attachment 286452 [details] [diff] [review]
v10

Could assert in GET_FUNCTION_PRIVATE that the slot is not JSVAL_VOID. Worthwhile? r/a=me either way, thanks.

/be
Attachment #286452 - Flags: review?(brendan)
Attachment #286452 - Flags: review+
Attachment #286452 - Flags: approval1.9+
Attached patch v10b (obsolete) — Splinter Review
The new version just fixes \ indentation in STOBJ_GET_PRIVATE:

#define STOBJ_SET_SYSTEM(obj)   ((void)((obj)->fslots[JSSLOT_CLASS] |= 2))
 
-#define STOBJ_GET_PRIVATE(obj)                                          \
+#define STOBJ_GET_PRIVATE(obj)                                                \
     (JS_ASSERT(JSVAL_IS_INT(STOBJ_GET_SLOT(obj, JSSLOT_PRIVATE))),            \
      JSVAL_TO_PRIVATE(STOBJ_GET_SLOT(obj, JSSLOT_PRIVATE)))
 
(In reply to comment #21)
> (From update of attachment 286452 [details] [diff] [review])
> Could assert in GET_FUNCTION_PRIVATE that the slot is not JSVAL_VOID.

This is already done in OBJ_GET_PRIVATE macro, see above.
Attachment #286452 - Attachment is obsolete: true
Attachment #286470 - Flags: review+
Attachment #286470 - Flags: approval1.9+
The last patch shrinks an optimized build of jsshell from 575440 to 572944 or by 0.4%. 
I checked in the patch from comment 22 to CVS trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1194965700&maxdate=1194965785&who=igor%mir2.org
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
I backed out the check-in as it caused failures in mochi tests.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Here is a test case for the problem exposed by the mochi test:

function f(m) {
    m = m || Math;
    return (function(){
        return m.sin(0);
    })();
};

var r = f(1);
if (r !== Math.sin(0))
    throw "Unexpected result";
~/m/trunk/mozilla/js/src $ ./Linux_All_DBG.OBJ/js ~/m/y.js
/home/igor/m/y.js:4: TypeError: m.sin is not a function
The previous test is wrong, here is a proper one that passes without the patch and with the patch it gives:

~/m/trunk/mozilla/js/src $ cat ~/m/y.js
function f(m) {
    m = m || Math;
    return x();

    function x() {
        return m.sin(0);
    }
};

var r = f();
if (r !== Math.sin(0))
    throw "Unexpected result";
~/m/trunk/mozilla/js/src $ js ~/m/y.js
/home/igor/m/y.js:6: TypeError: m is undefined
Attached patch v11 (obsolete) — Splinter Review
The patch caused by wrong code transformation in call_resolve from jsfun.c. Before the patch the essential sequence was:

            if (getter == js_GetArgument) {
                vp = fp->argv;
                nslots = JS_MAX(fp->argc, fp->fun->nargs);
...      
            }
            if (slot < nslots) {
                value = vp[slot];
...
            } else {
                value = JSVAL_VOID;
...
            }

Since the patch ensures that slot < fp->fun->nargs, I tries to simplify based on that. For some reasons I misread that JS_MAX as JS_MIN and transformed the code into:

value = (slot < fp->argc) ? fp->argv[slot] : JSVAL_VOID;

where the right way would be to write just
 
value = fp->argv[slot];

or with even simpler code as done in the new version of the patch.
Attachment #286470 - Attachment is obsolete: true
Attachment #288541 - Flags: review?(brendan)
Bugzilla inter-diff shows the right changes in jsfun.c between v11 abd v10b. The changes reported for other files are artifacts of unrelated commits. Here for references that full proper interdiff:

+++ js/src/jsfun.c	2007-11-13 22:54:26.000000000 +0200
@@ -813,17 +813,17 @@ call_resolve(JSContext *cx, JSObject *ob
              JSObject **objp)
 {
     JSStackFrame *fp;
     JSString *str;
     JSAtom *atom;
     JSLocalKind localKind;
     JSPropertyOp getter, setter;
     uintN slot, attrs, spflags;
-    jsval value;
+    jsval *vp;
 
     fp = (JSStackFrame *) JS_GetPrivate(cx, obj);
     if (!fp)
         return JS_TRUE;
     JS_ASSERT(fp->fun);
 
     if (!JSVAL_IS_STRING(id))
         return JS_TRUE;
@@ -836,34 +836,34 @@ call_resolve(JSContext *cx, JSObject *ob
     atom = js_AtomizeString(cx, str, 0);
     if (!atom)
         return JS_FALSE;
 
     localKind = js_LookupLocal(cx, fp->fun, atom, &slot);
     if (localKind != JSLOCAL_NONE) {
         if (localKind == JSLOCAL_ARG) {
             JS_ASSERT(slot < fp->fun->nargs);
-            value = (slot < fp->argc) ? fp->argv[slot] : JSVAL_VOID;
+            vp = fp->argv;
             getter = setter = NULL;
             attrs = JSPROP_PERMANENT;
             spflags = 0;
             slot = 0;
         } else {
             JS_ASSERT(localKind == JSLOCAL_VAR || localKind == JSLOCAL_CONST);
             JS_ASSERT(fp->fun->u.i.nvars == fp->nvars);
             JS_ASSERT(slot < fp->nvars);
-            value = fp->vars[slot];
+            vp = fp->vars;
             getter = js_GetCallVariable;
             setter = js_SetCallVariable;
             attrs = (localKind == JSLOCAL_CONST)
                     ? JSPROP_PERMANENT | JSPROP_READONLY
                     : JSPROP_PERMANENT;
             spflags = SPROP_HAS_SHORTID;
         }
-        if (!js_DefineNativeProperty(cx, obj, ATOM_TO_JSID(atom), value,
+        if (!js_DefineNativeProperty(cx, obj, ATOM_TO_JSID(atom), vp[slot],
                                      getter, setter, attrs,
                                      spflags, (int) slot, NULL)) {
             return JS_FALSE;
         }
         *objp = obj;
         return JS_TRUE;
     }
 
 
Comment on attachment 288541 [details] [diff] [review]
v11

The check in for bug 397289 required to update this patch.
Attachment #288541 - Attachment is obsolete: true
Attachment #288541 - Flags: review?(brendan)
Attached patch v12 (obsolete) — Splinter Review
Besides syncing with the CVS head, the new version also removes bogus slot = 0 assignment in call_resolve that I added in the previous patch in an attempt to fix the regression.
Attachment #288678 - Flags: review?(brendan)
Attached patch v13 (obsolete) — Splinter Review
The previous version passes slot as a shortid even when it is not used in the argument case. The new version adds explicit shortid for that.
Attachment #288678 - Attachment is obsolete: true
Attachment #288679 - Flags: review?(brendan)
Attachment #288678 - Flags: review?(brendan)
No longer blocks: 398219
Attachment #288679 - Flags: review?(brendan)
Attachment #288679 - Flags: review+
Attachment #288679 - Flags: approval1.9+
I checked in the patch from comment 32 to CVS trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1195432553&maxdate=1195432592&who=igor%mir2.org
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
I backed out the patch due to more mochitest failures.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
In the patch I missed the fact that null as a getter/setter passed to js_DefineNativeProperty in call_resolve means to default to the getter/setter of the class that relies on short id machinery. Here is a regression test to test the regression:

function test(memo) {
    doit();
    return memo;

    function doit() {
        memo = memo * 2;
    }
}

var result = test(1);
if (result !== 2)
    throw "Unexpected result:"+uneval(result);
Attached patch v14Splinter Review
Attachment #289310 - Flags: review?(brendan)
Attachment #288679 - Attachment is obsolete: true
Comment on attachment 289310 [details] [diff] [review]
v14

Sorry, I should have spotted that shortid = 0.

/be
Attachment #289310 - Flags: review?(brendan)
Attachment #289310 - Flags: review+
Attachment #289310 - Flags: approval1.9+
I checked in the patch from comment 36 to the CVS trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1195492398&maxdate=1195492560&who=igor%mir2.org
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Is this expected to break existing code?  An extension that I use broke from this.  So I'm wondering if I need to notify the author or file a bug.  
(In reply to comment #40)
> Is this expected to break existing code?  An extension that I use broke from
> this.  So I'm wondering if I need to notify the author or file a bug.  

What exactly is the problem?

When closing a tab the closed tab's contents are still shown.  The tab is removed from the tab bar.  And there is no active tab in the tab bar.  

The error in console when closing a tab is: 
Error: uncaught exception: [Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIDOMXULElement.removeChild]"  nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame :: chrome://allinonegest/content/gestimp.js :: removeTab :: line 226"  data: no] 

Looking in that file of the extension I see this:

  aioContent.removeTab = function(aTab) {
    if (aTab.localName != "tab") aTab = aioContent.mCurrentTab;
    var lBrowser = aioContent.getBrowserForTab(aTab);
    var ds = lBrowser.docShell;
    if (ds.contentViewer && !ds.contentViewer.permitUnload()) return;
    var tabObj = {};
    tabObj.next = aioSetTabId(aTab.nextSibling);
    tabObj.myId = aioSetTabId(aTab);
    tabObj.hist = lBrowser.sessionHistory;
    aioLastTabInfo.push(tabObj);
    if (aioLastTabInfo.length > 5) aioLastTabInfo.shift();
    aioContent.aioNativeRemoveTab(aTab);
  }

which I'm guessing that might be relevant to problem.  
(In reply to comment #42)
> The error in console when closing a tab is: 
> Error: uncaught exception: [Exception... "Component returned failure code:
> 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIDOMXULElement.removeChild]" 
> nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame ::
> chrome://allinonegest/content/gestimp.js :: removeTab :: line 226"  data: no] 

Have you recompiled the browser build fully? One of the consequences of the bug is changes in the xdr format which is used to precompile scripts. Such changes should be transparent (it was done in past in quite a few cases) as a different version number for older xdr-images triggers their recompilation.
I'm using tinderbox hourly builds so AFAIK that means no to the question of doing a full rebuild.  So I 'll wait and see if the problem clears up in tomorrow's nightly build.  
Depends on: 404480
(In reply to comment #42)
> When closing a tab the closed tab's contents are still shown.  The tab is
> removed from the tab bar.  And there is no active tab in the tab bar.  
> 

Not sure if your problem is same as mine but I too am facing problems relating to closing Tabs. Details in Bug #404480.
The problem is still in the nightly build.  The extension is this one:
https://addons.mozilla.org/en-US/firefox/addon/12

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007112005 Minefield/3.0b2pre ID:2007112005
Depends on: 404499
Depends on: 404655
(In reply to comment #41)
> (In reply to comment #40)
> > Is this expected to break existing code?  An extension that I use broke from
> > this.  So I'm wondering if I need to notify the author or file a bug.  
> 
> What exactly is the problem?
> 

In my case I'm patching some functions of the PlacesController (supportsCommand, isCommandEnabled and doCommand). This is working fine with the 20071119_0736 build but broken in 20071119_0916.
Checkins between those two builds: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=1195486560&maxdate=1195492559&cvsroot=%2Fcvsroot

Your patch from comment #36 is one of them.

Here is an example of how I'm patching the functions.

eval('PlacesController.prototype.isCommandEnabled='+PlacesController.prototype.isCommandEnabled.toString().replace(
	'replcace_string',
	'replcace_string \
	new_parts')
);
(In reply to comment #47)
> In my case I'm patching some functions of the PlacesController
> (supportsCommand, isCommandEnabled and doCommand). This is working fine with
> the 20071119_0736 build but broken in 20071119_0916.

This should be fixed when the patch for bug 404499 is landed.
(In reply to comment #48)
> (In reply to comment #47)
> > In my case I'm patching some functions of the PlacesController
> > (supportsCommand, isCommandEnabled and doCommand). This is working fine with
> > the 20071119_0736 build but broken in 20071119_0916.
> 
> This should be fixed when the patch for bug 404499 is landed.
> 

That sounds great. :) I'll test your patch in the meantime.
Blocks: 404935
Note to self: This test tests the regression not the original bug.

Checking in regress-398609.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-398609.js,v  <--  regress-398609.js
initial revision: 1.1
Flags: in-testsuite+
Flags: in-litmus-
Depends on: 406769
regression verified fixed 2007-12-09 1.9.0 linux|mac|win
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.