398609 - Simpler handling of hidden properties

Status: VERIFIED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

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.

Comment 1

[Igor Bukanov]

Attachment: v1

This is a backup of a patch that passes the test suite for the shell. The browser tests come next.

Comment 2

[Igor Bukanov]

Attachment: v2

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.

Comment 3

[Brendan Eich [:brendan]]

Will look when I can -- want a compiled code size win, did you get one?

/be

Comment 4

[Igor Bukanov]

(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.

Comment 5

[Igor Bukanov]

Attachment: v3

The new version simplifies logic in BindNameToSlot from jsemit.c

Comment 6

[Igor Bukanov]

Attachment: v4

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.

Comment 7

[Igor Bukanov]

Attachment: v5

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.

Comment 8

[Igor Bukanov]

Attachment: v6

The new version uses null as a setter for hidden properties eliminating js_SetArgument and js_SetLocalVariable functions.

Comment 9

[Igor Bukanov]

Attachment: v7

The new version inlines js_LookupPropertywithFlags in js_LookupName to make js_LookupName infallible.

Comment 10

[Igor Bukanov]

(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.

Comment 11

[Igor Bukanov]

Attachment: v8

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.

Comment 12

[Igor Bukanov]

Attachment: v8b

This is the previous version syncronized with CVS trunk changes.

Comment 13

[Brendan Eich [: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

Comment 14

[Igor Bukanov]

(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.

Comment 15

[Igor Bukanov]

Attachment: v9

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.

Comment 16

[Igor Bukanov]

Attachment: v9b

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.

Comment 17

[Igor Bukanov]

Attachment: diff between v8b and v9b

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.

Comment 18

[Igor Bukanov]

Attachment: diff between v8b and v9b for real

The previous diff is v9b itself, not the interdiff.

Comment 19

[Igor Bukanov]

(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...

Comment 20

[Igor Bukanov]

Attachment: v10

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.

Comment 21

[Brendan Eich [: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

Comment 22

[Igor Bukanov]

Attachment: v10b

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.

Comment 23

[Igor Bukanov]

The last patch shrinks an optimized build of jsshell from 575440 to 572944 or by 0.4%. 

Comment 24

[Igor Bukanov]

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

Comment 25

[Igor Bukanov]

I backed out the check-in as it caused failures in mochi tests.

Comment 26

[Igor Bukanov]

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

Comment 27

[Igor Bukanov]

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

Comment 28

[Igor Bukanov]

Attachment: v11

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.

Comment 29

[Igor Bukanov]

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 30

[Igor Bukanov]

Comment on attachment 288541 [details] [diff] [review]
v11

The check in for bug 397289 required to update this patch.

Comment 31

[Igor Bukanov]

Attachment: v12

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.

Comment 32

[Igor Bukanov]

Attachment: v13

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.

Comment 33

[Igor Bukanov]

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

Comment 34

[Igor Bukanov]

I backed out the patch due to more mochitest failures.

Comment 35

[Igor Bukanov]

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);

Comment 36

[Igor Bukanov]

Attachment: v14

Comment 37

[Igor Bukanov]

Attachment: v13-v14 interdiff

Comment 38

[Brendan Eich [:brendan]]

Comment on attachment 289310 [details] [diff] [review]
v14

Sorry, I should have spotted that shortid = 0.

/be

Comment 39

[Igor Bukanov]

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

Comment 40

[Brian Polidoro]

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.  

Comment 41

[Igor Bukanov]

(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?

Comment 42

[Brian Polidoro]

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.  

Comment 43

[Igor Bukanov]

(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.

Comment 44

[Brian Polidoro]

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.  

Comment 45

[ronin.achilles]

(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.

Comment 46

[Brian Polidoro]

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

Comment 47

[Ronny Perinke]

(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')
);

Comment 48

[Igor Bukanov]

(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.

Comment 49

[Ronny Perinke]

(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.

Comment 50

[Bob Clary [:bc] (inactive)]

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

Comment 51

[Bob Clary [:bc] (inactive)]

regression verified fixed 2007-12-09 1.9.0 linux|mac|win