300079 - XUL chrome precompiled functions don't inherit from current window's Function.prototype

Status: VERIFIED FIXED

Description

[ARNOU]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; fr-FR; rv:1.7.8) Gecko/20050511 Firefox/1.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; fr-FR; rv:1.7.8) Gecko/20050511 Firefox/1.0.4

When I try to implement rico.js and prototype.js in a XUL page, without use
their functions, this error appear 

this._mouseUpHandler.bindAsEventListener is not a function
Fichier Source : chrome://tests/content/rico.js
Ligne : 1301

This error is not generated with an HTML document... where is the difference ?

Reproducible: Always

Steps to Reproduce:
1.include scripts prototype.js and rico.js in an empty xul doc 
2.call this xul page in your favorite browser


Actual Results:  
the error "... is not a function" appear

Expected Results:  
no error, nothing else

Comment 1

[Bob Clary [:bc:]]

not a js engine bug

Comment 2

[Fritz Schneider]

Simpler test case:

Function.prototype.foo = "bar";
function f(){}
alert(f.foo);

Alerts "undefined" when executed in chrome (extension).

Comment 3

[Brendan Eich [:brendan]]

This sounds like an uninteded XPCNativeWrapper automation effect, if it affects
JS prototype and instance behavior in one and only one window.  If it affects
how content script can or cannot see properties set by chrome script where the
chrome manifest does not say xpcnativewrappers=no, then it's expected and
desired secure behavior, new in 1.5.

In the latter event, the way to get to the content object is via the
wrappedJSObject property of the XPCNativeWrapper -- or if you are *sure* it's
safe to do so, to set xpcnativewrappers=no in the chrome manifest.

See http://developer.mozilla.org/en/docs/XPCNativeWrapper for docs and feel free
to help fix them if you are able.

/be

Comment 4

[Brendan Eich [:brendan]]

If this is all within one window (as it sounds like -- attaching a testcase
would cinch it), then this sounds like something broke in "brutal sharing",
which for a long time has precompiled JS functions once only, sharing their
scripts among N windows loading the XUL via cloned function objects.

A cloned functoin object f' delegates to its prototype f, the clone-parent.  And
f's prototype is the object referenced by Function.prototype when the script in
which f is declared or expressed was compiled.

I only now noticed the report was against 1.0.4.  Is this broken also in Firefox
1.5b1?  Argh.  Any idea when this broke since Firefox 1.0 (when I believe, or at
least hope, it worked)?

/be

Comment 5

[Martin Honnen]

Here is some XUL test case run from HTTP (not chrome):
<http://home.arcor.de/martin.honnen/mozillaBugs/xul/FunctionPrototypeTest1.xul>

The test script used is here:
<http://home.arcor.de/martin.honnen/mozillaBugs/xul/FunctionPrototypeTest1.js>

A HTML test case using the same script is here:
<http://home.arcor.de/martin.honnen/mozillaBugs/xul/FunctionPrototypeTest1.html>

Output for the XUL test case with Firefox 1.5 beta nightly (Mozilla/5.0
(Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050912 Firefox/1.4):

Function.prototype.GOD: Kibo
f.GOD: undefined
f instanceof Function: false

f.constructor: function Function() {
    [native code]
}

f.constructor.GOD: undefined


Output for XUL test case with Mozilla suite 1.7.11 release (Mozilla/5.0
(Windows; U; Windows NT 5.1; en-US; rv:1.7.11) Gecko/20050728)

Function.prototype.GOD: Kibo
f.GOD: undefined
f instanceof Function: true

f.constructor: 
function Function() {
    [native code]
}

f.constructor.GOD: undefined


So the only difference between Mozilla 1.7.11 and Firefox 1.5 beta 1 nightly is
that 'instanceof Function' gives true in the former while it yields false in the
latter. The property added to Function.prototype is not inherited at all in both
builds.

For HTML everything is the same in both builds and the prototype based
inheritance is there:

Function.prototype.GOD: Kibo
f.GOD: Kibo
f instanceof Function: true

f.constructor: function Function() {
    [native code]
}

f.constructor.GOD: Kibo

Comment 6

[Martin Honnen]

I have now tried the XUL test case with Mozilla 1.0 (Mozilla/5.0 (Windows; U;
Windows NT 5.1; en-US; rv:1.0.0) Gecko/20020530) where the result is already
showing that properties added to Function.prototype are not inherited to functions:

Function.prototype.GOD: Kibo
f.GOD: undefined
f instanceof Function: true

f.constructor: 
function Function() {
    [native code]
}

f.constructor.GOD: undefined

Comment 7

[Martin Honnen]

Further tests with:

- Mozilla 1.1 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.1)
Gecko/20020826)
- Mozilla 1.2.1 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.2.1)
Gecko/20021130)
- Mozilla 1.4 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4)
Gecko/20030624)
- Mozilla 1.5 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5)
Gecko/20031007)
- Mozilla 1.6 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6)
Gecko/20040113)
- Mozilla 1.7 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7)
Gecko/20040616)

all show the same result for the XUL document:

Function.prototype.GOD: Kibo
f.GOD: undefined
f instanceof Function: true

f.constructor: 
function Function() {
    [native code]
}

f.constructor.GOD: undefined


So as far as I see that currently adding properties to Function.prototype in
script in XUL documents has never made those properties available to functions
defined in the script.

Comment 8

[Brendan Eich [:brendan]]

Yup, this goes back to 2001 or so, the dawn of "brutal sharing" (precompilation
of XUL scripts, including their functions).  There is a Function.prototype per
chrome window, but functions precompiled against a dummy singleton scope object,
or more recently against null, do not delegate to it.

To fix this we would need to do some JS engine magic.

/be

Comment 9

[Phil Ringnalda (:philor)]

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

Comment 10

[Brendan Eich [:brendan]]

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

Comment 11

[Brendan Eich [:brendan]]

Bug 321231 complained about instanceof changing from Firefox 1 to 1.5, but as comment 5 et seq. show, instanceof lied in Firefox 1.

This is arguably a bug in XUL's use of the JS API, but we'll need API extensions to fix it, so I'm leaving it in Core / JavaScript Engine.

/be

Comment 12

[Arpad Borsos [:Swatinem]]

Are there any new information on this bug? Is there any hope this could be fixed for 1.9?

Comment 13

[Brendan Eich [:brendan]]

This will be fixed for 1.9.

/be

Comment 14

[Brendan Eich [:brendan]]

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

Comment 15

[Chris Lacaze]

I tested the following on a js script loaded over HTTP by a remote XUL page:

Function.prototype.test = function() { alert('Test'); }
testme = function() { }
testme.test();

The test method was not invoked so it seems that this bug also happens for remote XUL pages (not Chrome). On an HTML page it worked fine. A similar test with Object also works fine.

Has it been fixed in 1.9a? If not, when can we expect a fix? Thanks.

Comment 16

[Boris Zbarsky [:bzbarsky, bz on IRC]]

If it had been fixed, it would have been resolved fixed.

And it might not get fixed in 1.9, though drivers would like to try to find resources somewhere to deal with it -- see status whiteboard and blocking flags.

Comment 17

[Brendan Eich [:brendan]]

I'll give it a go.

/be

Comment 19

[Brendan Eich [:brendan]]

Ok, working on this.

/be

Comment 20

[Brendan Eich [:brendan]]

Created attachment 268622 [details] [diff] [review]
proposed fix

The essential fix here is the one-line change to js_CloneFunctionObject. The rest is hygiene inspired by bug 383269 and highlighted by this patch's change to cloned function object prototyping.

Since with this patch, a cloned function object no longer holds onto its "clone-parent" function object via its proto slot, this patch changes fun_trace to trace fun->object unconditionally. This may run into problems with the cycle collector -- see bug 375999 and bug 375808 before it, both still open.

Testing needed, I wanted to throw this out for review and others' testing.

/be

Comment 21

[Brendan Eich [:brendan]]

I should move ATOM_OFFSET and OFFSET_TO_ATOM to jsatom.h and remove the originals in jsapi.c, rather than dup'ing those two into jsfun.c.

/be

Comment 22

[Igor Bukanov]

I will review the patch tomorrow. 

Comment 23

[Brendan Eich [:brendan]]

Created attachment 268631 [details] [diff] [review]
proposed fix, with jsatom.h common macros

Comment 24

[Brendan Eich [:brendan]]

Created attachment 268691 [details] [diff] [review]
proposed fix, with untagging optimization in fun_resolve

No need for JSVAL_TO_STRING from id or from ATOM_KEY(atom).

/be

Comment 25

[Brendan Eich [:brendan]]

Created attachment 268710 [details] [diff] [review]
proposed fix, with same-object-principal check on parentProto

This patch adds an important trust-boundary check before making f'.prototype.__proto__ == f.prototype for cloned function object f' and clone-parent f.

/be

Comment 26

[Brendan Eich [:brendan]]

Comment on attachment 268710 [details] [diff] [review]
proposed fix, with same-object-principal check on parentProto

This is wrong, of course: without the proto link to the clone-parent, call_resolve can't find hidden-atom-named properties for formals and locals to reflect. More work in fun_resolve is needed.

/be

Comment 27

[Brendan Eich [:brendan]]

Created attachment 268719 [details] [diff] [review]
proposed fix, with correct hidden property resolution

Comment 28

[Brendan Eich [:brendan]]

Comment on attachment 268719 [details] [diff] [review]
proposed fix, with correct hidden property resolution

This works in my Minefield build. Running the js/tests suite now.

/be

Comment 29

[Igor Bukanov]

Comment on attachment 268719 [details] [diff] [review]
proposed fix, with correct hidden property resolution

jsregexp.c changes are unrelated to the bug and it will be nice to move them to a separated bug. r+ with that.

Comment 30

[Brendan Eich [:brendan]]

Created attachment 268751 [details] [diff] [review]
proposed fix, about to commit

js/tests pass with known failures (AFAICT).

jsregexp.c patch split out as bug 384846.

/be

Comment 31

[Brendan Eich [:brendan]]

Fixed on trunk:

js/src/jsapi.c 3.328
js/src/jsatom.h 3.62
js/src/jsfun.c 3.190

Tony, IIRC some support JS for Safe Browsing works around this bug. It should not need to in 1.9/Fx3.

/be

Comment 32

[Boris Zbarsky [:bzbarsky, bz on IRC]]

I backed this out.  This had the salutary effect of fixing the crash on our leak tinderbox that was preventing us from gettting leak numbers and keeping anyone from checking in (8 hours after this was checked in, too).

Unfortunately, the tinderbox stack has the usual symbol issue; if someone has access to the tbox they could try running it through fix-linux-stack.pl on there...

Comment 33

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Applying the patch to one of my debug builds gives me a consistent shutdown crash due to us trying to log a release on the mRawPtr of an nsCOMPtr<nsIAtom>.  The vtable pointer of said mRawPtr is 0xdadadada.

The stack is compatible with the crash tinderbox is seeing.

Comment 34

[Brendan Eich [:brendan]]

Sorry, had to be out of town early till late today.

Will investigate -- I didn't see that shutdown crash, FWIW.

/be

Comment 35

[Brendan Eich [:brendan]]

(In reply to comment #34)
> Will investigate -- I didn't see that shutdown crash, FWIW.

I thought I tested all the way thru shutdown, but I must not have -- sorry again.

So this patch seems to cause the last GC to happen after XPCOM shutdown, during which the Gecko static atoms have been nuked. Details in an attachment next, but it's scary that we have this fragile shutdown logic. Old news, I suppose.

/be

Comment 36

[Brendan Eich [:brendan]]

Created attachment 268914 [details]
gdb chicken scratching

Comment 37

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Benjamin, see comment 35 and comment 36...  We really shouldn't have stuff alive this late in shutdown unless we basically leaked, right?

Comment 38

[Brendan Eich [:brendan]]

Created attachment 268922 [details] [diff] [review]
proposed fix, with #if 0'd FIXME-commented bit in fun_trace

Comments more than welcome. I'd like to re-land this as is and get to the bottom of the issues raised in the comment in a followup bug (which might be one of the bugs cited there).

/be

Comment 39

[Brendan Eich [:brendan]]

(In reply to comment #37)
> Benjamin, see comment 35 and comment 36...  We really shouldn't have stuff
> alive this late in shutdown unless we basically leaked, right?

There was in fact a leak due to the tracing of the fun->object "back pointer".

/be

Comment 40

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Hmm...  Should we be clearing the prototype cache early on in shutdown?  Would that help?

Comment 41

[Benjamin Smedberg]

It's not clear from the comments above which atoms we're talking about, and what "last GC after XPCOM shutdown" means. If we're talking about something in the prototype cache being held after the layout module has shut down, that probably means we need to hold onto nsLayoutStatics from the object that's being kept alive.

What side effects does clearing the prototype cache have?

Comment 42

[Boris Zbarsky [:bzbarsky, bz on IRC]]

These are the Gecko static atoms (nsGkAtomList), as far as I can tell....

Comment 43

[Brendan Eich [:brendan]]

(In reply to comment #41)
> It's not clear from the comments above which atoms we're talking about, and
> what "last GC after XPCOM shutdown" means.

From the attachment:

#3  0x2c012169 in NS_PurgeAtomTable () at nsAtomTable.cpp:376

and:

#12 0x2303e716 in js_GC (cx=0x22c308c0, gckind=GC_LAST_CONTEXT) at jsgc.c:2982
#13 0x23017952 in js_DestroyContext (cx=0x22c308c0, mode=JSDCM_FORCE_GC) at jscntxt.c:409
#14 0x23008a2e in JS_DestroyContext (cx=0x22c308c0) at jsapi.c:984
#15 0x12036581 in XPCJSContextStack::~XPCJSContextStack (this=0x32eaa0) at xpcthreadcontext.cpp:61
#16 0x12035947 in XPCPerThreadData::Cleanup (this=0x32ea60) at xpcthreadcontext.cpp:483
#17 0x1203716b in XPCPerThreadData::~XPCPerThreadData (this=0x32ea60) at xpcthreadcontext.cpp:492
#18 0x1203726c in xpc_ThreadDataDtorCB (ptr=0x32ea60) at xpcthreadcontext.cpp:527
#19 0x2000a25e in _PR_DestroyThreadPrivate (self=0x3070c0) at prtpd.c:265
#20 0x200206f1 in _pt_thread_death (arg=0x3070c0) at ptthread.c:825
#21 0x200209bb in _PR_Fini () at ptthread.c:999
#22 0x8fe0e33d in __dyld__ZN16ImageLoaderMachO13doTerminationERKN11ImageLoader11LinkContextE ()
#23 0x8fe030ec in __dyld__ZN4dyld14runTerminatorsEv ()
#24 0x00002c74 in cxa_atexit_wrapper ()

> If we're talking about something in
> the prototype cache being held after the layout module has shut down, that
> probably means we need to hold onto nsLayoutStatics from the object that's
> being kept alive.

Why would anything DOM-ish want to outlive XPCOM shutdown? Sure, leaks cause that, but it's a situation we might hope to rectify with more work. For instance if the DOM code knows XPCOM is shutting down, and it knows windows will survive that event, perhaps it could put them in some kind of safe mode. At the least, any static atoms managed by XPCOM would have to be safely nulled, or else also survive -- but these are xpcom/ds-hosted atoms, so how can XPCOM shutdown yet let them live?

You may know of bugs on making leaks across shutdown less harmful, or otherwise improving the situation. Obviously we should fix all shutdown-surviving leaks, but my question is: can we do more to mitigate their effects?

> What side effects does clearing the prototype cache have?

Not clear. Could have bad effects per the FIXME in the latest patch here, if a function prototype goes away before a cloned function object is activated in a (presumably leaking) chrome window. Does XPCOM shutdown guarantee that no more chrome scripts or event handlers can run?

/be

Comment 44

[Brendan Eich [:brendan]]

(In reply to comment #43)
> but my question is: can we do more to mitigate their effects?

I should have said "mitigate their effects in release builds?" In debug builds, we should have assertions botching (warnings in the short run if too many to fix for a clean tinderbox with assertions fatal) as soon as possible. Ideally at XPCOM shutdown if there are identifiable leaks.

/be

Comment 45

[Brendan Eich [:brendan]]

Fixed fix landed:

js/src/jsapi.c 3.330
js/src/jsatom.h 3.64
js/src/jsfun.c 3.192

/be

Comment 46

[Benjamin Smedberg]

> #18 0x1203726c in xpc_ThreadDataDtorCB (ptr=0x32ea60) at
> xpcthreadcontext.cpp:527
> #19 0x2000a25e in _PR_DestroyThreadPrivate (self=0x3070c0) at prtpd.c:265

ok, that's a serious xpconnect bug... it should be clearing the threadprivates during xpcom-shutdown-threads or perhaps in its module shutdown. I've seen similar bugs before that I thought were fixed. Timeless?

And it turns out the rest of my ramble about layout shutdown is irrelevant, because layout isn't actually involved at all.

> (presumably leaking) chrome window. Does XPCOM shutdown guarantee that no more
> chrome scripts or event handlers can run?

At some point late in the sequence, that is considered an invariant, yes. Doesn't mean we actually follow the rules.

Comment 47

[Blake Kaplan (:mrbkap) (inactive, use needinfo)]

Had to back this out again -- fun->object was being collected too early, causing us not to find local variables and arguments on the scope chain.

Comment 48

[Boris Zbarsky [:bzbarsky, bz on IRC]]

I'm hoping we'll check in a test for that with the next version?

Comment 49

[Brendan Eich [:brendan]]

Third time's the charm.

We don't have js shell tests for this embedding-specific bug. The JS component loader compiles and executes (why does it not just evaluate? Oh well) a script, throwing away the script and its atoms after it has been executed for effects against the global for the component's location. That means the prototype function objects are liable to be GC'ed. Unlike the XUL prototype cache, which hangs onto scripts, the JS component loader exercises a different pattern of API usage.

I can write a test that uses the clone() js-shell function to test directly for this bug.

/be

Comment 50

[Brendan Eich [:brendan]]

Created attachment 269033 [details]
js shell test for JS component loader use-case

Comment 51

[Brendan Eich [:brendan]]

Created attachment 269034 [details]
js shell test for JS component loader use-case

Comment 52

[Brendan Eich [:brendan]]

Created attachment 269035 [details] [diff] [review]
proposed fix, with fun_trace correctly tracing fun->object

Don't trace fun->object if it's null or if it is the same as obj, the function object on which fun_trace was called. This passes the js shell test and Minefield testing including clean shutdown.

This may fix the problem encountered with the cycle collector in bug 375999.

/be

Comment 53

[Brendan Eich [:brendan]]

Created attachment 269038 [details] [diff] [review]
tighter logic: fun->object can't be null in fun_resolve or fun_trace

Comment 54

[Boris Zbarsky [:bzbarsky, bz on IRC]]

It doesn't have to be a js shell test; we can exercise the XUL proto cache via mochitest, right?

Comment 55

[Brendan Eich [:brendan]]

(In reply to comment #54)
> It doesn't have to be a js shell test; we can exercise the XUL proto cache via
> mochitest, right?

The XUL prototype cache protects scripts whose literals include prototypal function objects, so there's no failure case there to test.

The js shell test is the most direct and has fewest dependencies. A JS component test could be added on top, but there's no reason to prefer it to the equivalent js shell test.

/be

Comment 56

[Brendan Eich [:brendan]]

Once more, with feeling -- or unto the breach, as Henry V put it:

js/src/jsapi.c 3.332
js/src/jsatom.h 3.66
js/src/jsfun.c 3.194

Igor's review is welcome tomorrow and I'll file followup bugs if needed. I want to get this into builds ASAP, with mrbkap's r+.

This was painful, but in hindsight there's no way to cheat Mother-GC and I should not have tried. We really do need to trace fun->object. But we do not want to trace the object currently being traced (this is different from the old mark hook, where the mark bit would be set on that object so that any self-mark attempts by its mark hook would immediately return).

/be

Comment 57

[Bob Clary [:bc:]]

/cvsroot/mozilla/js/tests/js1_8/extensions/regress-300079.js,v  <--  regress-300079.js
initial revision: 1.1

Comment 58

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This seems to have caused orange (shutdown crashes) on qm-xserve01 and qm-rhel02. Possibly also fxdbug-linux-tbox.

Comment 59

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

Tree is closed due to orange.

Comment 60

[Brendan Eich [:brendan]]

fxdbug-linux-tbox has non symbols, but this crash at shutdown stack:

  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/libxul.so +0x00058EC8]
  UNKNOWN [/lib/tls/libpthread.so.0 +0x0000B898]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x000FC977]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x0050D6A2]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x0050D6B5]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x0050D6FC]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x0050D7DE]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x00539ABA]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x00539ADD]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x00539AFF]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x005567F5]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x0055680C]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x005492D4]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x00563A40]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x00576C9B]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x000D7F66]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x000D80E4]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x0060BD79]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x006178A9]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x006091AF]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x006091E5]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/components/libxpconnect.so +0x000435BB]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x005DEC31]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/libmozjs.so +0x0005D3FB]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/libmozjs.so +0x0002B46A]
  JS_DestroyContext+0x00000019 [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/libmozjs.so +0x0001873A]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/components/libxpconnect.so +0x0004A5A5]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/components/libxpconnect.so +0x0004991D]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/components/libxpconnect.so +0x00049AAF]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/components/libxpconnect.so +0x00049B9B]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/libnspr4.so +0x000142B8]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/libnspr4.so +0x000311DA]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/libnspr4.so +0x00031553]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/libnspr4.so +0x00008E70]
  UNKNOWN [/builds/tinderbox/Fx-Trunk-Debug/Linux_2.6.9-42.ELsmp_Depend/mozilla/obj/dist/bin/libnspr4.so +0x00036C1A]
  UNKNOWN [/lib/ld-linux.so.2 +0x0000C907]
  exit+0x00000077 [/lib/tls/libc.so.6 +0x0002A527]
  __libc_start_main+0x000000DD [/lib/tls/libc.so.6 +0x00014DED]

looks like the one in attachment 268914 [details].

So there's an XPCOM/JS leak that defers last GC until after XPCOM has shut down and purged atoms stashed in statics by XSLT code, or similar.

This is a bug I'd like to fix directly instead of backing out again. More in a few minutes (by 10am PDT).

/be

Comment 61

[Brendan Eich [:brendan]]

I tried landing this patch:

Index: jsfun.c
===================================================================
RCS file: /cvsroot/mozilla/js/src/jsfun.c,v
retrieving revision 3.194
diff -p -u -8 -r3.194 jsfun.c
--- jsfun.c     20 Jun 2007 06:37:15 -0000      3.194
+++ jsfun.c     20 Jun 2007 17:01:46 -0000
@@ -1508,17 +1508,17 @@ fun_hasInstance(JSContext *cx, JSObject 
 static void
 fun_trace(JSTracer *trc, JSObject *obj)
 {
     JSFunction *fun;
 
     fun = (JSFunction *) JS_GetPrivate(trc->context, obj);
     if (fun) {
         JS_CALL_TRACER(trc, fun, JSTRACE_FUNCTION, "private");
-        if (fun->object != obj)
+        if (IS_GC_MARKING_TRACER(trc) && fun->object != obj)
             JS_CALL_TRACER(trc, fun->object, JSTRACE_OBJECT, "object");
         if (fun->atom)
             JS_CALL_TRACER(trc, fun->atom, JSTRACE_ATOM, "atom");
         if (FUN_INTERPRETED(fun) && fun->u.i.script)
             js_TraceScript(trc, fun->u.i.script);
     }
 }
 
as this revision:

js/src/jsfun.c 3.195

Hoping it helps, will back out to re-open tree otherwise.

/be

Comment 62

[Robert Sayre]

(In reply to comment #61)
> 
> Hoping it helps, will back out to re-open tree otherwise.

No longer crashing on shutdown, but we are now leaking a bunch instead (Rlk spike).

Comment 63

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Please back this out so we can reopen the tree.

Comment 64

[Brendan Eich [:brendan]]

(In reply to comment #63)
> Please back this out so we can reopen the tree.

Tried to ping you on IRC. This comment seems stale because the tree was already open at its timestamp. I'd like to get some time to work on the leak bug(s) that may be biting RLk. If that's ok with you we can let this bug stay closed for now.

I will file a bug on the RLk jump unless there's already one on file that someone can point out here.

/be

Comment 65

[Robert Sayre]

(In reply to comment #64)
> 
> I will file a bug on the RLk jump unless there's already one on file that
> someone can point out here.

I backed this out because roc needs to land the new textframe code, and we shouldn't do that with Rlk above 800k. Sorry. :/

Comment 66

[Brendan Eich [:brendan]]

Sayrer: sorry, didn't see you did indeed back out this patch plus the one-line change to jsfun.c -- thanks for that. Reopening.

/be

Comment 67

[Brendan Eich [:brendan]]

The change in comment 61 fixed orange tinderboxen, so there's clearly something to-do with the cycle collector (the only other user of the JS GC tracing API) that makes for shutdown leaks, which result in crashes due to Gecko XPCOM atoms that are memoized in statics being dereferenced after XPCOM shutdown has purged atoms.

That's bug #1.

Bug #2 is that with the hack in comment 61, RLk jumped. This suggests that there are uncollected XPCOM/JS cycles involving fun->object that happen not to result in bloat-test crashes, "merely" in leaks. Catch-22.

More after a breather. I am not going to check in without running all the bloat tests first.

/be

Comment 68

[Blake Kaplan (:mrbkap) (inactive, use needinfo)]

Created attachment 269116 [details] [diff] [review]
(Failed) Component loader patch

Brendan and I tried this patch to try to keep the function atoms alive longer (which we hoped would fix the bustage caused by attachment 268922 [details] [diff] [review]) but it didn't work.

Comment 69

[Brendan Eich [:brendan]]

Comment on attachment 269116 [details] [diff] [review]
(Failed) Component loader patch

This is really the wrong approach, just for the record. We wanted to try a workaround that made JS components work like the XUL prototype cache.

The last patch, which landed and was backed out (not including the patch in comment 61) is right. Each function must have a strong reference to the original (clone-parent) compiler-created function object.

There are bugs blocking this bug from landing. They'll be filed as needed, marked blocking, and fixed. After that, the last js/src patch will land again, and I'd bet money it will land unchanged except to merge up to trunk tip.

/be

Comment 70

[timeless]

>> #18 0x1203726c in xpc_ThreadDataDtorCB (ptr=0x32ea60) at
>> xpcthreadcontext.cpp:527
>> #19 0x2000a25e in _PR_DestroyThreadPrivate (self=0x3070c0) at prtpd.c:265

> ok, that's a serious xpconnect bug... it should be clearing the threadprivates
> during xpcom-shutdown-threads or perhaps in its module shutdown. I've seen
> similar bugs before that I thought were fixed. Timeless?

The general idea is that for any non main thread you do want xpconnect to clear its things at thread exit. there is no other time/place to do it, because if the objects were created on a certain thread and aren't thread safe, they really need to be destroyed on that thread. Note that this code is all ancient, if you guys have added any events in the past 5 years, that's all long after this code was written.

-- and yes, I know that currently xpconnect and gc don't behave properly wrt gc'ing threadunsafe objects, i had work related to that in a tree back in the states, I'm not sure whether I can find that or if I'll have to reimplement it, it wasn't a very big priority for me then and atm it really isn't --

iirc for a while nspr wasn't thread exiting the main thread (i think that might have been fixed relatively recently, I'm having some trouble keeping track of all the fixes people are doing).

But yes, it's probably a good idea for xpconnect to at module shutdown clear things (need to be careful here of course, but hey).

Comment 71

[Brendan Eich [:brendan]]

Re-landed:

js/src/jsapi.c 3.334
js/src/jsatom.h 3.68
js/src/jsfun.c 3.197

Of course, while testing and trying to debug, I pulled on a string and ended up updating my tree with a bunch of recent changes, one or more of which fixed the latent cycle-collector-related leak.

No significant changes to fxdbug-linux-tbox's leak stats (the A number wandered up to a previous high water mark from before my landing). Wish I knew what fixed the shutdown leak this patch exposed, but if it stays fixed, we can move on and spend energy finding unfixed bugs. (However, I'm a little suspicious that my luck won't hold.)

/be

Comment 72

[Bob Clary [:bc:]]

verified fixed 1.9.0 linux/mac*/windows.

Comment 73

[Brendan Eich [:brendan]]

The test for this bug is

js1_8/extensions/regress-300079.js

and it does use expression closures, but it really is testing the JS_CloneFunctionObject API, which has constraints I hadn't thought through when I developed the bug 452498 patches. Ulp.

Anyway, test should probably use ES3-conformant function syntax and move to an API tests subdir, if we have one, or else whatever version (js1_x) in which we added JS_CloneFunctionObject to the API (JS1.5 IIRC -- check me).

/be

Comment 74

[Bob Clary [:bc:]]

http://hg.mozilla.org/tracemonkey/rev/001292143fde
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-300079.js,v  <--  regress-300079.js
initial revision: 1.1
Removing js/tests/js1_8/extensions/regress-300079.js;
/cvsroot/mozilla/js/tests/js1_8/extensions/regress-300079.js,v  <--  regress-300079.js
new revision: delete; previous revision: 1.1