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

VERIFIED FIXED in mozilla1.9alpha6

Status

()

Core
JavaScript Engine
P2
normal
VERIFIED FIXED
13 years ago
9 years ago

People

(Reporter: ARNOU, Assigned: brendan)

Tracking

({helpwanted})

Trunk
mozilla1.9alpha6
helpwanted
Points:
---
Bug Flags:
blocking1.9 -
wanted1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 9 obsolete attachments)

11.42 KB, patch
brendan
: review+
Details | Diff | Splinter Review
5.77 KB, text/plain
Details
270 bytes, text/plain
Details
11.54 KB, patch
Igor Bukanov
: review+
mrbkap
: review+
Details | Diff | Splinter Review
(Reporter)

Description

13 years ago
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

13 years ago
not a js engine bug
Assignee: general → nobody
Component: JavaScript Engine → XP Toolkit/Widgets: XUL
QA Contact: general → xptoolkit.xul

Comment 2

13 years ago
Simpler test case:

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

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

Comment 3

13 years ago
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
Status: UNCONFIRMED → NEW
Component: XP Toolkit/Widgets: XUL → XP Miscellany
Ever confirmed: true
(Assignee)

Comment 4

13 years ago
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

13 years ago
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

13 years ago
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

13 years ago
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.
(Assignee)

Comment 8

13 years ago
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
Assignee: nobody → brendan
Component: XP Miscellany → JavaScript Engine
OS: Windows 2000 → All
Hardware: PC → All
*** Bug 313411 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 10

12 years ago
*** Bug 321231 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 11

12 years ago
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
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Updated

12 years ago
Summary: In XUL, a js function don't inherit from Function Object (and its prototype properties) → In XUL, a precompiled function doesn't inherit from the current window's Function object
Are there any new information on this bug? Is there any hope this could be fixed for 1.9?
(Assignee)

Comment 13

12 years ago
This will be fixed for 1.9.

/be
Flags: blocking1.9a2?
Flags: blocking1.9a2? → blocking1.9-
Whiteboard: [wanted-1.9]
(Assignee)

Comment 14

12 years ago
*** Bug 350655 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

12 years ago
Summary: In XUL, a precompiled function doesn't inherit from the current window's Function object → XUL chrome precompiled functions don't inherit from current window's Function.prototype

Comment 15

11 years ago
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.
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.
Keywords: helpwanted
(Assignee)

Comment 17

11 years ago
I'll give it a go.

/be
(Assignee)

Updated

11 years ago
Duplicate of this bug: 383201
(Assignee)

Comment 19

11 years ago
Ok, working on this.

/be
Target Milestone: mozilla1.9alpha1 → mozilla1.9alpha6
(Assignee)

Comment 20

11 years ago
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
Attachment #268622 - Flags: review?(igor)
(Assignee)

Comment 21

11 years ago
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

11 years ago
I will review the patch tomorrow. 
(Assignee)

Comment 23

11 years ago
Created attachment 268631 [details] [diff] [review]
proposed fix, with jsatom.h common macros
Attachment #268622 - Attachment is obsolete: true
Attachment #268631 - Flags: review?(igor)
Attachment #268622 - Flags: review?(igor)
(Assignee)

Comment 24

11 years ago
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
Attachment #268631 - Attachment is obsolete: true
Attachment #268691 - Flags: review?(igor)
Attachment #268631 - Flags: review?(igor)
(Assignee)

Comment 25

11 years ago
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
Attachment #268691 - Attachment is obsolete: true
Attachment #268710 - Flags: review?(igor)
Attachment #268691 - Flags: review?(igor)
(Assignee)

Updated

11 years ago
Attachment #268710 - Flags: review?(mrbkap)
(Assignee)

Comment 26

11 years ago
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
Attachment #268710 - Attachment is obsolete: true
Attachment #268710 - Flags: review?(mrbkap)
Attachment #268710 - Flags: review?(igor)
(Assignee)

Comment 27

11 years ago
Created attachment 268719 [details] [diff] [review]
proposed fix, with correct hidden property resolution
Attachment #268719 - Flags: review?(igor)
(Assignee)

Comment 28

11 years ago
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
Attachment #268719 - Flags: review?(mrbkap)

Comment 29

11 years ago
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.
(Assignee)

Comment 30

11 years ago
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
Attachment #268719 - Attachment is obsolete: true
Attachment #268751 - Flags: review+
Attachment #268719 - Flags: review?(mrbkap)
Attachment #268719 - Flags: review?(igor)
(Assignee)

Comment 31

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
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...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
(Assignee)

Comment 34

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

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

/be
Status: REOPENED → ASSIGNED
(Assignee)

Comment 35

11 years ago
(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
(Assignee)

Comment 36

11 years ago
Created attachment 268914 [details]
gdb chicken scratching
Benjamin, see comment 35 and comment 36...  We really shouldn't have stuff alive this late in shutdown unless we basically leaked, right?
(Assignee)

Comment 38

11 years ago
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
Attachment #268922 - Flags: review?(igor)
(Assignee)

Comment 39

11 years ago
(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

Updated

11 years ago
Attachment #268922 - Flags: review?(igor) → review+
Hmm...  Should we be clearing the prototype cache early on in shutdown?  Would that help?

Comment 41

11 years ago
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?
These are the Gecko static atoms (nsGkAtomList), as far as I can tell....
(Assignee)

Comment 43

11 years ago
(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
(Assignee)

Comment 44

11 years ago
(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
(Assignee)

Comment 45

11 years ago
Fixed fix landed:

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

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Comment 46

11 years ago
> #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.
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I'm hoping we'll check in a test for that with the next version?
(Assignee)

Comment 49

11 years ago
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
Status: REOPENED → ASSIGNED
(Assignee)

Comment 50

11 years ago
Created attachment 269033 [details]
js shell test for JS component loader use-case
(Assignee)

Comment 51

11 years ago
Created attachment 269034 [details]
js shell test for JS component loader use-case
Attachment #269033 - Attachment is obsolete: true
(Assignee)

Comment 52

11 years ago
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
Attachment #268922 - Attachment is obsolete: true
Attachment #269035 - Flags: review?(igor)
(Assignee)

Updated

11 years ago
Attachment #269035 - Flags: review?(mrbkap)
(Assignee)

Comment 53

11 years ago
Created attachment 269038 [details] [diff] [review]
tighter logic: fun->object can't be null in fun_resolve or fun_trace
Attachment #269035 - Attachment is obsolete: true
Attachment #269038 - Flags: review?(igor)
Attachment #269035 - Flags: review?(mrbkap)
Attachment #269035 - Flags: review?(igor)
(Assignee)

Updated

11 years ago
Attachment #269038 - Flags: review?(mrbkap)
It doesn't have to be a js shell test; we can exercise the XUL proto cache via mochitest, right?
(Assignee)

Comment 55

11 years ago
(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

Updated

11 years ago
Attachment #269038 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 56

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Attachment #269038 - Flags: review?(igor) → review+

Comment 57

11 years ago
/cvsroot/mozilla/js/tests/js1_8/extensions/regress-300079.js,v  <--  regress-300079.js
initial revision: 1.1
Flags: in-testsuite+
This seems to have caused orange (shutdown crashes) on qm-xserve01 and qm-rhel02. Possibly also fxdbug-linux-tbox.
Tree is closed due to orange.
(Assignee)

Comment 60

11 years ago
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
(Assignee)

Comment 61

11 years ago
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

11 years ago
(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).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Please back this out so we can reopen the tree.
(Assignee)

Comment 64

11 years ago
(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
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Comment 65

11 years ago
(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. :/
(Assignee)

Comment 66

11 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 67

11 years ago
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

Status: REOPENED → ASSIGNED
(Assignee)

Updated

11 years ago
Attachment #269038 - Attachment description: tigher logic: fun->object can't be null in fun_resolve or fun_trace → tighter logic: fun->object can't be null in fun_resolve or fun_trace
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.
(Assignee)

Comment 69

11 years ago
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
Attachment #269116 - Attachment is obsolete: true

Comment 70

11 years ago
>> #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).
(Assignee)

Comment 71

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Comment 72

11 years ago
verified fixed 1.9.0 linux/mac*/windows.
Status: RESOLVED → VERIFIED

Updated

10 years ago
Depends on: 408002
Flags: wanted1.9+
Whiteboard: [wanted-1.9]
(Assignee)

Comment 73

9 years ago
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

9 years ago
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
You need to log in before you can comment on or make changes to this bug.