When JS errors show up in the JS console, we should show the stack trace (like Chrome does -- note that Chrome seems to only do it if you have the console open).

ping.. we desperately need this for partner porting efforts. We're having to tell people to use Chrome to figure out where their exceptions/assertions are coming from -- debugger isn't useful, because we're talking about 100MB+ JS scripts. For bonus points -- an option to capture a stack trace on every console.log/error/etc. as well would be amazing.

not ideal, but one work-around would be to instrument known error locations with console.trace() commands. It will drop a stack trace into the console from that location. If you're really lucky, setting a window.onError handler to drop a console.trace() might also work. We're looking into this, but it requires some back-end changes to nsIScriptError and nsIConsoleMessage to make it work.

nsIScriptErrors come without any stacktrace. The interface needs to be updated accordingly, then we'll need to update code throughout Gecko wherever new nsIScriptErrors are generated to also include the correct stacktrace (where applicable). For console API it's simpler: we already call Components.stack which gives us access to the entire stacktrace. We only look at the youngest frame for performance reasons - walking through the entire stackframes is slow - see bug 762995. We need a bug about adding the complete stacktrace to console API messages, and another bug for the Gecko changes related to nsIScriptErrors. Then we can fix/update the Web Console's output to display the stacks.

bug 493414 might be of interest, there's a prototype patch extending nsIScriptError and implementing a few stack providers. (For the reference: bug 125612 is about the same feature in the Error Console.)

I've been playing around with the JavaScript Debugger API and crafted a little script to show stack traces https://github.com/DavidBruant/usefulStackTrace Not ready for production use. Maybe leaky, maybe it hurts performance a bit, but it gets part of the job done. The one thing it doesn't do is catching errors happening before you run it. I'll see if I can pack that in an add-on so that: 1) no need to run it after each refresh. 1.1) it catches all errors, even those happening very early (In reply to Rob Campbell [:rc] (:robcee) from comment #2) > We're looking into this, but it requires some back-end changes to > nsIScriptError and nsIConsoleMessage to make it work. Would it make sense to ship a script like the one I wrote so that people can have stack traces as soon as possible?

(In reply to David Bruant from comment #7) > Would it make sense to ship a script like the one I wrote so that people can > have stack traces as soon as possible? Making an add-on out of it could be useful, but we can't ship something like this in the browser, since using the Debugger API has a significant performance hit.

(In reply to Panos Astithas [:past] from comment #8) > (In reply to David Bruant from comment #7) > > Would it make sense to ship a script like the one I wrote so that people can > > have stack traces as soon as possible? > > Making an add-on out of it could be useful, but we can't ship something like > this in the browser, since using the Debugger API has a significant > performance hit. Even if this only runs with devtools on?

Yes, because then the perception would be that our tools slow down web page performance.

Let's say I'm on it even if I'm not sure my c++ knowledges are good enough to accomplish this. But I'm trying to hack something, at very least I'll describe what is going on at platform level. Note that there is quite a lot of duplicate to this bug over various components (devtools, core, toolkit, ...)

It looks like I managed to make something that works, but I would need a serious review regarding the JS/Xpconnect tweaks I made! https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4623d88ce3a

This bug is most likely going to fix various existing bugs like: 493414, 125612, 804772

(In reply to Alexandre Poirot [:ochameau] from comment #17) > It looks like I managed to make something that works, but I would need a > serious review regarding the JS/Xpconnect tweaks I made! > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4623d88ce3a Nick has worked on the C++ side of stacks recently with SavedFrame work. He may at least know the correct reviewers.

Comment on attachment 8594900 [details] [diff] [review] Pass SavedFrame from JS::ErrorObject to nsIScriptError. r=? Review of attachment 8594900 [details] [diff] [review]: ----------------------------------------------------------------- bholley or jorendorff would be good reviewers for this. Additionally, I have some questions/concerns below. ::: js/src/jsapi.h @@ +4569,5 @@ > unsigned errorNumber; /* the error number, e.g. see js.msg */ > const char16_t* ucmessage; /* the (default) error message */ > const char16_t** messageArgs; /* arguments for the error message */ > int16_t exnType; /* One of the JSExnType constants */ > + JS::Heap<JSObject*> stack; Nit: comment saying that it must be an object that is<SavedFrame>() or is a wrapper to such an object. (At least, thats what I would assume it must be) ::: js/xpconnect/src/nsScriptError.cpp @@ +160,5 @@ > *result = ToNewCString(mCategory); > return NS_OK; > } > > +JS::Value nsScriptError::wrapSavedFrame(JSContext *cx, JS::HandleObject frame) { Why not just use the default wrapper to the SavedFrame object? All the JSAPI methods for working with SavedFrame handle wrappers around SavedFrame objects just fine. This approach really feels dangerous to me, since we went to a lot of trouble to make sure we got the security of SavedFrame + wrappers + JS properties / JSAPI methods correct and it wasn't easy. See the comment at the top of vm/SavedStacks.h. @@ +165,5 @@ > + RootedValue val(cx); > + RootedObject target(cx, JS_NewPlainObject(cx)); > + > + // Copy all the SavedFrame attributes > + if (JS_GetProperty(cx, frame, "source", &val) && val.isString()) { Assuming there is some reason we can't just user wrappers around SavedFrame objects directly, why not use `JS::GetSavedFrameX` methods here?

New try with a destructors and some more assertions: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6616561c9eba (In reply to Nick Fitzgerald [:fitzgen] from comment #20) > Comment on attachment 8594900 [details] [diff] [review] > ::: js/xpconnect/src/nsScriptError.cpp > @@ +160,5 @@ > > *result = ToNewCString(mCategory); > > return NS_OK; > > } > > > > +JS::Value nsScriptError::wrapSavedFrame(JSContext *cx, JS::HandleObject frame) { > > Why not just use the default wrapper to the SavedFrame object? All the JSAPI > methods for working with SavedFrame handle wrappers around SavedFrame > objects just fine. If I just return mStack from nsScriptError::GetStack, I get some compartment mismatched assertions when accessing `parent` attribute. I tried loads of things. wrapSavedFrame was the only thing that works... JS_WrapValue/JS_WrapObject both wrap the frame but not it's parent frame object... > > This approach really feels dangerous to me, since we went to a lot of > trouble to make sure we got the security of SavedFrame + wrappers + JS > properties / JSAPI methods correct and it wasn't easy. See the comment at > the top of vm/SavedStacks.h. Note that this API is only for chrome, but I would be happy to do something different... I just haven't found anything simplier that works. > > @@ +165,5 @@ > > + RootedValue val(cx); > > + RootedObject target(cx, JS_NewPlainObject(cx)); > > + > > + // Copy all the SavedFrame attributes > > + if (JS_GetProperty(cx, frame, "source", &val) && val.isString()) { > > Assuming there is some reason we can't just user wrappers around SavedFrame > objects directly, why not use `JS::GetSavedFrameX` methods here? I wish I wouldn't have to do any of this!

I would prefer doing something like this, but it assert with mismatched compartments when I try to access `parent` attribute: nsScriptError::GetStack(JS::MutableHandleValue aStack) { AutoSafeJSContext cx; JSAutoCompartment ac(cx, &mStack.toObject()); aStack.set(mStack); return NS_OK; }

(In reply to Alexandre Poirot [:ochameau] from comment #21) > New try with a destructors and some more assertions: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=6616561c9eba > > (In reply to Nick Fitzgerald [:fitzgen] from comment #20) > > Comment on attachment 8594900 [details] [diff] [review] > > ::: js/xpconnect/src/nsScriptError.cpp > > @@ +160,5 @@ > > > *result = ToNewCString(mCategory); > > > return NS_OK; > > > } > > > > > > +JS::Value nsScriptError::wrapSavedFrame(JSContext *cx, JS::HandleObject frame) { > > > > Why not just use the default wrapper to the SavedFrame object? All the JSAPI > > methods for working with SavedFrame handle wrappers around SavedFrame > > objects just fine. > > If I just return mStack from nsScriptError::GetStack, I get some compartment > mismatched assertions when accessing `parent` attribute. > I tried loads of things. wrapSavedFrame was the only thing that works... > JS_WrapValue/JS_WrapObject both wrap the frame but not it's parent frame > object... > I think you can avoid these by wrapping the SavedFrame object into the target compartment with JS_WrapObject or JS_WrapValue: https://dxr.mozilla.org/mozilla-central/source/js/src/jsapi.cpp#993 See also JS::GetSavedFrameParent https://dxr.mozilla.org/mozilla-central/source/js/src/jsapi.h#5317 and the comments explaining the JSAPI functions for interacting with SavedFrame objects: https://dxr.mozilla.org/mozilla-central/source/js/src/jsapi.h#5241-5263

(In reply to Alexandre Poirot [:ochameau] from comment #22) So this example would become like this: nsScriptError::GetStack(JS::MutableHandleValue aStack) { AutoSafeJSContext cx; JSAutoCompartment ac(cx, myTargetCompartment); // Ensure that the stack object is in this compartment. RootedObject wrapped(cx, &mStack.toObject()); if (!JS_WrapObject(cx, &wrapped)) return NS_SOME_KIND_OF_ERROR; aStack.set(wrapped); return NS_OK; } However, it seems that whoever is getting the stack knows which compartment to wrap into, and the getter wouldn't really know that, so this might not be the right place to wrap, but wrapping is the general idea.

I think this issue with such wrapping is that it will only wrap the first frame. It won't wrap its `parent` attribute recursively, is it?

I get the following stack, with mismatched compartment with code similar to comment 24, with this for the compartment: JSAutoCompartment ac(cx, CurrentGlobalOrNull(cx)); #0 0x00007ffff3b74fd5 in js::CompartmentChecker::fail (c1=<optimized out>, c2=<optimized out>) at /mnt/desktop/gecko/js/src/jscntxtinlines.h:49 #1 0x00007ffff3b75153 in check (c=<optimized out>, this=0x7fffffff1320) at /mnt/desktop/gecko/js/src/jscntxtinlines.h:70 #2 check (obj=<optimized out>, this=0x7fffffff1320) at /mnt/desktop/gecko/js/src/jscntxtinlines.h:81 #3 js::CompartmentChecker::check (this=0x7fffffff1320, v=...) at /mnt/desktop/gecko/js/src/jscntxtinlines.h:101 #4 0x00007ffff3c54498 in assertSameCompartment<JS::MutableHandle<JS::Value> > (t1=..., cx=0x7ffff6b73840) at /mnt/desktop/gecko/js/src/jscntxtinlines.h:162 #5 js::CallJSNative (cx=0x7ffff6b73840, native=0x7ffff3c9f420 <js::SavedFrame::parentProperty(JSContext*, unsigned int, JS::Value*)>, args=...) at /mnt/desktop/gecko/js/src/jscntxtinlines.h:237 #6 0x00007ffff3c45cb1 in js::Invoke (cx=cx@entry=0x7ffff6b73840, args=..., construct=construct@entry=js::NO_CONSTRUCT) at /mnt/desktop/gecko/js/src/vm/Interpreter.cpp:727 #7 0x00007ffff3c476fa in js::Invoke (cx=cx@entry=0x7ffff6b73840, thisv=..., fval=..., argc=argc@entry=0, argv=argv@entry=0x0, rval=..., rval@entry=...) at /mnt/desktop/gecko/js/src/vm/Interpreter.cpp:783

(In reply to Alexandre Poirot [:ochameau] from comment #25) > I think this issue with such wrapping is that it will only wrap the first > frame. > It won't wrap its `parent` attribute recursively, is it? JS::GetSavedFrameParent is *not* guaranteed to return a SavedFrame object that is in the same compartment. See comment here: https://dxr.mozilla.org/mozilla-central/source/js/src/jsapi.h#5311-5315 You need to explicitly wrap again.

So I get back to my wrapSavedFrame thing... I don't think I can do something simplier?

Comment on attachment 8594900 [details] [diff] [review] Pass SavedFrame from JS::ErrorObject to nsIScriptError. r=? Jason, Could you take a quick look at this patch? I'm not a great c++/jsapi/xpconnect hacker so I most likely did various things wrong, but at least, I would like to know if I'm on the right track. If you think I should ask to someone else, please forward to request! The overall goal is just to pass over the SavedFrame object up to the webconsole code. Today it ends up stuck in js::ErrorObject. In this patch I make it so that it is transfered up to nsIScriptError by also transfering it via JSErrorReport in the way.

(In reply to Alexandre Poirot [:ochameau] from comment #28) > So I get back to my wrapSavedFrame thing... I don't think I can do something > simplier? Ah, I think you have stumbled onto a bug in the SavedFrame::parentProperty method! I think it is failing to rewrap its return value. Can you try with this change in js/src/vm/SavedStacks.cpp: /* static */ bool SavedFrame::parentProperty(JSContext* cx, unsigned argc, Value* vp) { THIS_SAVEDFRAME(cx, argc, vp, "(get parent)", args, frame); RootedObject parent(cx); (void) JS::GetSavedFrameParent(cx, frame, &parent); + if (parent && !cx->compartment()->wrap(cx, &parent)) + return false; args.rval().setObjectOrNull(parent); return true; }

(In reply to Nick Fitzgerald [:fitzgen] from comment #30) > (In reply to Alexandre Poirot [:ochameau] from comment #28) > > So I get back to my wrapSavedFrame thing... I don't think I can do something > > simplier? > > Ah, I think you have stumbled onto a bug in the SavedFrame::parentProperty > method! Right! The patch is much much simplier now. I'm still leaking the universe, but I have no clue why/how. https://treeherder.mozilla.org/#/jobs?repo=try&revision=c39e17e480dc Also I'm wondering if I should root the object (use some cycle collector magic macros) on ErrorReport and JSErrorReport, like what I'm doing on nsScriptError...

Comment on attachment 8595887 [details] [diff] [review] Pass SavedFrame from JS::ErrorObject to nsIScriptError Bobby, I tried various cycle collection macros, but I can't havious the following (intermittent) crash, during one mochitest shutdown. IndexedDB codebase ends up instanciating a nsScriptError object from its background thread. Is NS_IMPL_CYCLE_COLLECTING_ADDREF(nsScriptError) threadsafe? Can it be called from a background thread, even the indexed DB one? It crashes within the ADDREF macro call. Here is the stack trace: #0 0x00007ffff15f7b59 in NS_CycleCollectorSuspect3 (aPtr=0x7fffcb4a4af0, aCp=0x0, aRefCnt=0x7fffcb4a4af8, aShouldDelete=0x0) at /mnt/desktop/gecko/xpcom/base/nsCycleCollector.cpp:3955 #1 0x00007ffff16216f9 in incr (aCp=0x0, aOwner=0x7fffcb4a4af8, aOwner@entry=0x7fffcb4a4b00, this=0x7fffcb4a4af8, this@entry=0x7fffcb4a4b00) at ../../dist/include/nsISupportsImpl.h:266 #2 nsCycleCollectingAutoRefCnt::incr (this=this@entry=0x7fffcb4a4af8, aOwner=aOwner@entry=0x7fffcb4a4af0) at ../../dist/include/nsISupportsImpl.h:252 #3 0x00007ffff1c8ba4c in nsScriptError::AddRef (this=this@entry=0x7fffcb4a4af0) at /mnt/desktop/gecko/js/xpconnect/src/nsScriptError.cpp:36 #4 0x00007ffff32354c6 in nsScriptErrorConstructor (aOuter=<optimized out>, aIID=..., aResult=0x7fffd93fea38) at /mnt/desktop/gecko/js/xpconnect/src/XPCModule.h:26 #5 0x00007ffff1644639 in nsComponentManagerImpl::CreateInstanceByContractID (this=<optimized out>, aContractID=<optimized out>, aDelegate=0x0, aIID=..., aResult=aResult@entry=0x7fffd93fea38) at /mnt/desktop/gecko/xpcom/components/nsComponentManager.cpp:1231 #6 0x00007ffff166c78a in CallCreateInstance (aContractID=<optimized out>, aDelegate=<optimized out>, aIID=..., aResult=aResult@entry=0x7fffd93fea38) at /mnt/desktop/gecko/xpcom/glue/nsComponentManagerUtils.cpp:151 #7 0x00007ffff166c7ce in nsCreateInstanceByContractID::operator() (this=0x7fffd93fea90, aIID=..., aInstancePtr=0x7fffd93fea38) at /mnt/desktop/gecko/xpcom/glue/nsComponentManagerUtils.cpp:197 #8 0x00007ffff1caafd5 in nsCOMPtr<nsIScriptError>::assign_from_helper (this=this@entry=0x7fffd93fea78, helper=..., aIID=...) at ../../../dist/include/nsCOMPtr.h:1116 #9 0x00007ffff1fe1718 in nsCOMPtr (aHelper=..., this=0x7fffd93fea78) at ../../dist/include/nsCOMPtr.h:531 #10 nsContentUtils::LogSimpleConsoleError (aErrorText=..., classification=classification@entry=0x7ffff435e968 "indexedDB") at /mnt/desktop/gecko/dom/base/nsContentUtils.cpp:3312 #11 0x00007ffff2c4dc18 in mozilla::dom::indexedDB::ReportInternalError (aFile=<optimized out>, aFile@entry=0x7ffff44b889b "/mnt/desktop/gecko/dom/indexedDB/ActorsParent.cpp", aLine=aLine@entry=11539, aStr=aStr@entry=0x7ffff44b8996 "UnknownErr") at /mnt/desktop/gecko/dom/indexedDB/ReportInternalError.cpp:31 Otherwise, I'm wondering if that's because of this change on nsIScriptError interface: - NS_DECL_THREADSAFE_ISUPPORTS + NS_DECL_CYCLE_COLLECTING_ISUPPORTS + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(nsScriptError) Are we still as threadsafe as before if using the cycle collecting macros?

I'm pretty sure that cycle-collected objects can't have threadsafe refcounting (someone please correct me if I'm wrong). AddRef/Release need to call into the global CC tables, which is totally unsafe to do on multiple threads. Naively, it would make sense to me to make nsScriptError main-thread-only, and use nsMainThreadPtrHolder in any places where we need to hold references to it off-main-thread. I don't know how extensively it's used in those situations though. bent might have a better sense.

(In reply to Bobby Holley (:bholley) from comment #34) > I'm pretty sure that cycle-collected objects can't have threadsafe > refcounting (someone please correct me if I'm wrong). AddRef/Release need to > call into the global CC tables, which is totally unsafe to do on multiple > threads. Correct. > Naively, it would make sense to me to make nsScriptError main-thread-only We use that on worker threads too.

Ok - does workers need to use it cross-thread, or can we at least make it non-threadsafe (which would solve the issue here)?

Well, we have to make one to pass to the console service from any thread. Why is it holding cycle-collected objects?!

Oh I see. Yeah, if this is the object we pass the the console service, we can't have it hold onto cycle-collected objects. We should probably use a different object or subclass for that.

(A main-thread-only subclass would probably work well).

The lifetime of these objects isn't clear to me either. Doesn't the console service buffer and keep some of these objects alive for a while? Even if you have a main-thread-only subclass you still probably wouldn't want to put CC'd stuff in there I don't think.

Nick recently did a lot of work to be smart about saving stack frames, reducing a lot the memory footpring of it. Here, I'm "just" trying to pass a reference of this smart stack frames from jspi/xpconnect code up to the console code in JS. This object is a SavedFrame instance: http://mxr.mozilla.org/mozilla-central/source/js/src/vm/SavedStacks.h#16 which is a JSObject. I'm not an expert about lifecycle of object in gecko, but I imagine the only way to pass along this JSObject reference over these ErrorObject, nsScriptError abstraction is to mark these SavedFrame objects in the CC... That would be unfortunate if I have to redo all what Nick did just because this object is a JSObject. That to save the stack frames on nsScriptError without putting in the CC. Otherwise regarding console service, yes, we do buffer the messages: https://dxr.mozilla.org/mozilla-central/source/xpcom/base/nsConsoleService.cpp#198 We buffer the last 250 ones, while also notifying the nsIConsoleListener observers. This typically call the devtools JS codebase here: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/webconsole/utils.js#1246 The console service handle threads, it can be called from main thread or others and the console listener calls in another thread if called from the main one. (From what I understood with my limited c++ skills!) Here is a summary of the codepath: - js/src/vm/ErrorObject.cpp -- ErrorObject::init and ErrorObject::getOrCreateErrorReport In both cases we are creating JSErrorReport instances, we do have the SavedFrame object, we do set it on these JSErrorReport instances. JSErrorReport is defined in jsapi.h - js/xpconnect/src/nsXPConnect.cpp -- ErrorReport::Init We get called with the JSErrorReport instance, we set the SavedFrame on ErrorReport instances we are creating. ErrorReport is defined in xpcpublic.h - js/xpconnect/src/nsXPConnect.cpp -- ErrorReport::LogToConsole We are creating a nsScriptError instance, we do set the stack on it and add it there in the CC. Then we call nsConsoleService::LogMessage. nsScriptError is defined in xpcprivate.h

Ok yeah - I think we need a main-thread-only cycle-collected subclass and appropriate handling in nsConsoleService.cpp to make sure that we don't hold things alive indefinitely.

New version, with cycle collected, main thread only subclass. But it looks like I still have other kind of crashes... https://treeherder.mozilla.org/#/jobs?repo=try&revision=715c4d1c4830 ( Just submitted a new try with more checks in the destructor: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5dc01c8caedc ) Note that these crashes do not happen in regular tests (exception from a regular webpage) and are intermittent and hard to reproduce locally when running the crashing tests reported on try. This seems to be related to nsScriptErrorWithStack.mStack handling, as there is no crash if I prevent setting it with this test patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=61cef71c6dfd Any idea about what could be wrong now?

Oh! It looks like most of the crashes were due to the lack of checks in the destructor, the last try looks much better: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5dc01c8caedc I only get two of these crashes, not 100% sure it is related to this patch, but most likely: Assertion failure: CurrentThreadCanAccessZone(zone), at /builds/slave/try-l64-d-00000000000000000000/build/src/js/src/gc/Marking.cpp:188 Otherwise try is almost full orange because of leaks now. I imagine we keep manyyyy things alive as we store this object in the console storage. It's not clear how to handle that. Should I use some weakref mechanism to the stack object? (does that even exists in our gecko c++ world?)

Comment on attachment 8602731 [details] [diff] [review] Pass SavedFrame from JS::ErrorObject to nsIScriptError v2 Review of attachment 8602731 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsapi.h @@ +4568,5 @@ > const char16_t* ucmessage; /* the (default) error message */ > const char16_t** messageArgs; /* arguments for the error message */ > int16_t exnType; /* One of the JSExnType constants */ > + JS::Heap<JSObject*> stack; /* the complete stackframe where this error happened */ > + /* must be a SavedFrame JSObject or a wrapper of it */ This seems like a GC hazard. What keeps it alive? This may be the source of your problem. ::: js/xpconnect/idl/nsIScriptError.idl @@ +61,5 @@ > readonly attribute unsigned long long innerWindowID; > > readonly attribute boolean isFromPrivateWindow; > > + attribute jsval stack; Seems like this should go in an inherited interface, nsIScriptErrorWithStack, no? That would let us differentiate these more easily and avoid the need to define an empty stack getter/setter on nsScriptError. ::: js/xpconnect/src/nsScriptErrorWithStack.cpp @@ +49,5 @@ > +{ > +} > + > +nsScriptErrorWithStack::~nsScriptErrorWithStack() { > + if (!mStack.isUndefined() && mStack.isGCThing()) { The first condition is subsumed by the second. ::: js/xpconnect/src/nsXPConnect.cpp @@ +261,5 @@ > // there's no console service without affecting the other reporting > // mechanisms. > nsCOMPtr<nsIConsoleService> consoleService = > do_GetService(NS_CONSOLESERVICE_CONTRACTID); > + nsCOMPtr<nsIScriptError> errorObject = new nsScriptErrorWithStack(); Why not just pass the JSObject* to the constructor and make the XPIDL type read-only? ::: js/xpconnect/src/xpcprivate.h @@ +3028,5 @@ > +private: > + virtual ~nsScriptErrorWithStack(); > + // Complete stackframe where the error happened. > + // Must be SavedFrame object. > + JS::Heap<JS::Value> mStack; Given that, shouldn't we make this a JS::Heap<JSObject*>?

(In reply to Alexandre Poirot [:ochameau] from comment #44) > Otherwise try is almost full orange because of leaks now. > I imagine we keep manyyyy things alive as we store this object in the > console storage. Yes, I meant to mention above that this patch also needs to handle these things in the console service and avoid buffering them or holding them alive somehow. > It's not clear how to handle that. Should I use some weakref mechanism to > the stack object? (does that even exists in our gecko c++ world?) If you used a weakref, the stack would go away - there's nothing else holding it alive but the error report.

10.34 + ~ErrorReport() { 10.35 + mStack = nullptr; 10.36 + } Also, this is non-sensical. If this was fixing crashes, it's probably a red herring.

Here is a patch that prevent the leaks. That prune the cache of nsIScriptError(WithStack) objects store in nsConsoleService. This patch could be landed as-is, it doesn't depends on stack patches. https://treeherder.mozilla.org/#/jobs?repo=try&revision=22e3e46829fc

Still need to address all the last comments, I'm just prevent to set stacks on messages without InnerWindowID, so that the previous patch really allows to fix the leaks.

(In reply to Bobby Holley (:bholley) from comment #45) > Comment on attachment 8602731 [details] [diff] [review] > Pass SavedFrame from JS::ErrorObject to nsIScriptError v2 > > Review of attachment 8602731 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jsapi.h > @@ +4568,5 @@ > > const char16_t* ucmessage; /* the (default) error message */ > > const char16_t** messageArgs; /* arguments for the error message */ > > int16_t exnType; /* One of the JSExnType constants */ > > + JS::Heap<JSObject*> stack; /* the complete stackframe where this error happened */ > > + /* must be a SavedFrame JSObject or a wrapper of it */ > > This seems like a GC hazard. What keeps it alive? This may be the source of > your problem. Yes... I really wonder what happens here! I thought that JS::Heap was somehow magically adding reference to the JSObject based on the object lifetime itself. I imagine I would have to do something similar to CYCLE_COLLECTION macros I used in xpconnect codebase. But here, this code lives in js/src/ folder, where there is no usage of CYCLE_COLLECTION macros, do you happen to know how I could explicitely keep this ref alive here?

(In reply to Alexandre Poirot [:ochameau] from comment #50) > Yes... I really wonder what happens here! I thought that JS::Heap was > somehow magically adding reference to the JSObject based on the object > lifetime itself. > I imagine I would have to do something similar to CYCLE_COLLECTION macros I > used in xpconnect codebase. > But here, this code lives in js/src/ folder, where there is no usage of > CYCLE_COLLECTION macros, > do you happen to know how I could explicitely keep this ref alive here? Well, there are things like PersistentRooted, but right now JSErrorReport can't really be used for _any_ of these kinds of things, since it's malloc-ed, not new-ed. So you'll basically need to re-do JSErrorReport handling (see jsexn.cpp) or find a different way to pass the stack around. The latter is probably much easier.

Fixed various crash due to mistakes in ResetWindow... Almost green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb0c1fff7d2d I see only two errors: - Issues in jit2/Spidermonkey tests: This is weird, it looks like we run the same tests in different test suites?! And we don't know what is wrong... just some test failed. I don't know how to run these tests? - May be some failure in reftest (second run pending) TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/bugs/632781-verybig.html | application terminated with exit code -15 TEST-UNEXPECTED-FAIL | leakcheck | tab process: missing output line for total leaks! - Crash in mochitest-browser - browser/components/shell/test/browser_633221.js Assertion failure: mRawPtr != 0 (You can't dereference a NULL nsCOMPtr with operator->().), at ../../dist/include/nsCOMPtr.h:706 PROCESS-CRASH | Main app process exited normally | application crashed [@ nsGlobalWindow::FreeInnerObjects()] @@ -1566,6 +1567,10 @@ nsGlobalWindow::FreeInnerObjects() // Prune messages related to this window in the console cache + nsCOMPtr<nsIConsoleService> console(do_GetService("@mozilla.org/consoleservice;1")); + console->ResetWindow(mWindowID); May the console service not be available in some (shutdown) cases? Especially in child process as it only crashes in e10s.

Addressed all comments but nsIScriptErrorWithStack/JS::Heap on JSErrorReport. My last try was almost all green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=77b3880e88db The only failures lefts are: * random crashes in webaudio PROCESS-CRASH | dom/media/webaudio/test/test_audioBufferSourceNodeEnded.html | application crashed [@ js::gc::TenuredCell::arenaHeader() const] Not completely sure it is my patch, I launched another try on new tip: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eeedcaedd0ef * leak reported in mochitest-plain-2, only in e10s I don't fully understand why I had to add a `if (console)` in FreeInnerObjects(). On e10s it seems to be not (always) possible to fetch the console service and so, free the related messages...

So. I ended up trying what you suggested in comment 51. I'm now passing the stack JSObject later in the codeflow, from dom/base/nsJSEnvironment.cpp:SystemErrorReporter to nsXPConnect.cpp:ErrorReport. It ends up fixing most of the previous errors on previous try!! But there is still one assertion in the GC on just one test (dom/html/test/test_anchor_ping.html): Assertion failure: !aGCThing, at /builds/slave/try-l64-d-00000000000000000000/build/src/xpcom/base/CycleCollectedJSRuntime.cpp:937 #01: TraceCallbackFunc::Trace(JS::Heap<JSObject*>*, char const*, void*) const [xpcom/glue/nsCycleCollectionParticipant.cpp:101] #02: mozilla::CycleCollectedJSRuntime::AssertNoObjectsToTrace(void*) [xpcom/base/CycleCollectedJSRuntime.cpp:947] #03: nsCycleCollector::CollectWhite() [xpcom/base/nsCycleCollector.cpp:3266] #04: nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) [xpcom/base/nsCycleCollector.cpp:3622] #05: nsCycleCollector_collectSlice(js::SliceBudget&, bool) [tools/profiler/GeckoProfilerImpl.h:398] https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1a47348b25d http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/apoirot@mozilla.com-047809c6c9a5/try-linux64-debug/try_ubuntu64_vm-debug_test-mochitest-other-bm51-tests1-linux64-build307.txt.gz I don't know what's going on now. To me, there is no more GC hazard possible here. I'm no longer setting the stack object on any class that is not GC-traced. I'm only keeping a new reference on nsScriptErrorWithStack, which should be correct now. This patch is a bit messy in nsXPConnect, as I've let a previous version of that patch, that, instead of introducing LogToConsoleWithStack, was passing the stack object via Init(). But it could potentially introduce some GC hazard... but from what I've seen it doesn't make anything fail on try, just the same test, same assertion. Yet another try, just to be sure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1a47348b25d

Also, I tried hard to play with the various NS_ macros... but couldn't find a way to make things to work! It sounds so simple but so hard to do with the macros. nsScriptErrorWithStack -> nsScriptError -----------> nsIScriptError \-------> nsIScriptErrorWithStack ------^ But I've hit two issues: nsScriptErrorWithStack is a cycle collected class, whereas nsScriptError isn't. nsScriptErrotWithStack ends up inheriting from nsIScriptError/nsISupports two times via nsScriptError and nsIScriptErrorWithStack.

Comment on attachment 8612334 [details] [diff] [review] Pass SavedFrame from JS::ErrorObject to nsIScriptError - v5 Does that look any better?

Comment on attachment 8612334 [details] [diff] [review] Pass SavedFrame from JS::ErrorObject to nsIScriptError - v5 Review of attachment 8612334 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/nsXPConnect.cpp @@ +225,5 @@ > { > + LogToConsoleWithStack(nullptr); > +} > +void > +xpc::ErrorReport::LogToConsoleWithStack(JSObject* aStack) This is a GC hazard - you need to be passing a HandleObject here. You should enable the hazard analysis on your try push, which will tell you about these issues. @@ +268,5 @@ > + if (mWindowID && aStack) { > + // Only set stack on messages related to a document > + // As we cache messages in the console service, > + // we have to ensure not leaking them after the related > + // context is destroyed and we only track document lifecycle for now. What handling is this? I don't see it anywhere. ::: js/xpconnect/src/xpcprivate.h @@ +3016,5 @@ > bool mIsFromPrivateWindow; > }; > > +// Definition of nsScriptError, defined here because we lack a place to put > +// XPCOM objects associated with the JavaScript engine. You don't need to duplicate this comment. ::: js/xpconnect/src/xpcpublic.h @@ +514,2 @@ > void LogToConsole(); > + void LogToConsoleWithStack(JSObject* aStack); I don't get it - why are we both (a) storing the object (via Init()), and (b) passing it directly with LogToConsoleWithStack?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b88f09af45d4 (In reply to Bobby Holley (:bholley) from comment #58) > Comment on attachment 8612334 [details] [diff] [review] > Pass SavedFrame from JS::ErrorObject to nsIScriptError - v5 > > Review of attachment 8612334 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/xpconnect/src/nsXPConnect.cpp > @@ +225,5 @@ > > { > > + LogToConsoleWithStack(nullptr); > > +} > > +void > > +xpc::ErrorReport::LogToConsoleWithStack(JSObject* aStack) > > This is a GC hazard - you need to be passing a HandleObject here. You should > enable the hazard analysis on your try push, which will tell you about these > issues. Oh right, I tried to address that in this new patch! > > @@ +268,5 @@ > > + if (mWindowID && aStack) { > > + // Only set stack on messages related to a document > > + // As we cache messages in the console service, > > + // we have to ensure not leaking them after the related > > + // context is destroyed and we only track document lifecycle for now. > > What handling is this? I don't see it anywhere. This is in another patch, attachment 8608782 [details] [diff] [review], dedicated to cleanup message store from messages of dying documents. (See also comment 56 about my attempt to implement a nsIScriptErrorWithStack interface) > ::: js/xpconnect/src/xpcpublic.h > @@ +514,2 @@ > > void LogToConsole(); > > + void LogToConsoleWithStack(JSObject* aStack); > > I don't get it - why are we both (a) storing the object (via Init()), and > (b) passing it directly with LogToConsoleWithStack? This patch is still a WIP. I've let both approachs why only using (b) as it's less subject to GC hazard! xpc::ErrorReport isn't declared to the GC, so its `JS::Heap<JSObject*> mStack;` looks unsafe, right? Or is the fact that it is refcounted make it safe `NS_INLINE_DECL_THREADSAFE_REFCOUNTING(ErrorReport);`? Or it will be safe if I use PersistentRooted instead of Heap? I kept (a) as it "looks better", mostly because we already pass all similar argument during Init() and none durong LogToConsole.

(In reply to Alexandre Poirot [:ochameau] from comment #59) > This patch is still a WIP. I've let both approachs why only using (b) as > it's less subject to GC hazard! Having multiple approaches in the patch makes it difficult for me to give feedback. > xpc::ErrorReport isn't declared to the GC, so its `JS::Heap<JSObject*> > mStack;` looks unsafe, right? Or is the fact that it is refcounted make it > safe `NS_INLINE_DECL_THREADSAFE_REFCOUNTING(ErrorReport);`? If you hold a JS::Heap, your object needs to be cycle-collected. > Or it will be safe if I use PersistentRooted instead of Heap? You should not be using PersistentRooted for such things.

Comment on attachment 8608782 [details] [diff] [review] Cleanup console service cache when a window is destroyed https://treeherder.mozilla.org/#/jobs?repo=try&revision=65aad18269ea So, try is green and I think I addressed most of your comments now. I'm asking you to review, but do not hesitate to forward to whoever makes sense! This first patch, independent from the stack one, allows to prevent leaking the console messages once the document is gone. It is very important for console messages with stacks as stack object are JS object and keep many things alive...

Comment on attachment 8616785 [details] [diff] [review] Pass SavedFrame from JS::ErrorObject to nsIScriptError - v7 And the stack patch... that requires the previous one in order to not introduce zillions of leaks! This version is now cleaned up. I ended up keeping passing the stack object into LogToConsoleWithStack, that to prevent having to convert xpc::ErrorReport to a cycle-collected object (and do like all other params, pass them through the Init() method). Same thing, if you think someone else needs to review this, do not hesitate to forward!

Comment on attachment 8608782 [details] [diff] [review] Cleanup console service cache when a window is destroyed Review of attachment 8608782 [details] [diff] [review]: ----------------------------------------------------------------- Bouncing this to Nathan.

Comment on attachment 8616785 [details] [diff] [review] Pass SavedFrame from JS::ErrorObject to nsIScriptError - v7 Review of attachment 8616785 [details] [diff] [review]: ----------------------------------------------------------------- Getting there! Sorry for the delayed review. ::: dom/base/nsJSEnvironment.cpp @@ +427,5 @@ > } > > if (status != nsEventStatus_eConsumeNoDefault) { > + if (mError.isObject()) { > + ThreadsafeAutoJSContext cx; AutoJSContext is deprecated. Use AutoJSAPI and init it with mError. @@ +429,5 @@ > if (status != nsEventStatus_eConsumeNoDefault) { > + if (mError.isObject()) { > + ThreadsafeAutoJSContext cx; > + JS::RootedObject exObj(cx, mError.toObjectOrNull()); > + JSObject *stack = JS_StackFromException(cx, exObj); Nit: * on the left now, and don't leave bare pointers on the stack - this should go into the rooted directly. And as mentioned elsewhere, LogToConsoleWithStack should take a Handle<JSObject*>. ::: js/src/jsapi.cpp @@ +5614,5 @@ > +JS_StackFromException(JSContext* cx, JS::HandleObject obj) > +{ > + AssertHeapIsIdle(cx); > + CHECK_REQUEST(cx); > + AutoCompartment ac(cx, obj); Replace this with: assertSameCompartment(cx, obj); ::: js/src/jsapi.h @@ +4867,5 @@ > extern JS_PUBLIC_API(JSErrorReport*) > JS_ErrorFromException(JSContext* cx, JS::HandleObject obj); > > +extern JS_PUBLIC_API(JSObject*) > +JS_StackFromException(JSContext* cx, JS::HandleObject obj); Let's call this JS::ExceptionStackOrNull. ::: js/src/jsexn.cpp @@ +325,5 @@ > > +JSObject* > +js::StackFromException(JSContext* cx, HandleObject objArg) > +{ > + RootedObject obj(cx, UncheckedUnwrap(objArg)); This UncheckedUnwrap seems pretty unsafe to me. Unless there's a good reason, this should be CheckedUnwrap. ::: js/src/jsexn.h @@ +76,5 @@ > extern JSErrorReport* > ErrorFromException(JSContext* cx, HandleObject obj); > > +extern JSObject* > +StackFromException(JSContext* cx, HandleObject obj); I don't see a reason to have both a js:: and JS:: variant. Just make the version in jsexn.cpp the public one. ::: js/xpconnect/src/nsScriptErrorWithStack.cpp @@ +41,5 @@ > + NS_INTERFACE_MAP_ENTRY(nsIConsoleMessage) > + NS_INTERFACE_MAP_ENTRY(nsIScriptError) > +NS_INTERFACE_MAP_END > + > +nsScriptErrorWithStack::nsScriptErrorWithStack(JS::HandleValue aStack) nsScriptErrorWithStack is only safe if it has a windowID, given the console leak issue, right? I think it should override ::Init to MOZ_CRASH. ::: js/xpconnect/src/nsXPConnect.cpp @@ +220,5 @@ > > void > xpc::ErrorReport::LogToConsole() > { > + AutoJSContext cx; AutoJSContext is deprecated - use AutoJSAPI. @@ +221,5 @@ > void > xpc::ErrorReport::LogToConsole() > { > + AutoJSContext cx; > + RootedValue o(cx); You don't need this - you should be able to pass null directly, which constructs a sentinel handle. @@ +225,5 @@ > + RootedValue o(cx); > + LogToConsoleWithStack(o); > +} > +void > +xpc::ErrorReport::LogToConsoleWithStack(JS::HandleValue aStack) As mentioned elsewhere, this should be a HandleObject. ::: js/xpconnect/src/xpcprivate.h @@ +3022,5 @@ > + > + NS_DECL_CYCLE_COLLECTING_ISUPPORTS > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(nsScriptErrorWithStack) > + > + nsresult GetStack(JS::MutableHandleValue); This should be NS_IMETHOD @@ +3028,5 @@ > +private: > + virtual ~nsScriptErrorWithStack(); > + // Complete stackframe where the error happened. > + // Must be SavedFrame object. > + JS::Heap<JS::Value> mStack; Given that we know the type, this should be a JS::Heap<JSObject*>. ::: js/xpconnect/tests/chrome/test_nsScriptErrorWithStack.xul @@ +1,1 @@ > +<?xml version="1.0"?> Don't add new XUL chrome tests - gen_template.pl -type chrome outputs html now.

Comment on attachment 8608782 [details] [diff] [review] Cleanup console service cache when a window is destroyed Review of attachment 8608782 [details] [diff] [review]: ----------------------------------------------------------------- Apologies for the review delay. r=me with changes below. ::: xpcom/base/nsConsoleService.cpp @@ +85,5 @@ > + > + if (i == mBufferSize - 1) { > + // If we removed the last item, we just nullify the current slot > + mMessages[i] = nullptr; > + mCurrent = mBufferSize - 1; I think this is just the degenerate case of the loop below, so you can handle all the cases with just the loop? @@ +87,5 @@ > + // If we removed the last item, we just nullify the current slot > + mMessages[i] = nullptr; > + mCurrent = mBufferSize - 1; > + } else { > + // Now shift all the followings Nit: "Now shift all the following messages" @@ +96,5 @@ > + mMessages[j] = nullptr; > + mCurrent = j; > + } > + > + // The array it no longer full Nit: "The array is..." @@ +100,5 @@ > + // The array it no longer full > + mFull = false; > + > + // Proceed to the next message, which moved the the current idx > + // nullify the for's i++ Nit: Something like "Ensure the next iteration handles the messages we just shifted down." would be better. ::: xpcom/base/nsIConsoleService.idl @@ +47,5 @@ > + > + /** > + * Clear the message buffer for a given inner window. > + */ > + void resetWindow(in uint64_t innerWindowID); This is a complete nit, but I think clearMessagesForWindow (clearMessagesForWindowID?) would be much nicer. I know it's not symmetrical with |reset()|, but resetWindow() sounds very wrong.

Here is a try run with just this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfb619bf4831 (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #67) > Comment on attachment 8608782 [details] [diff] [review] > ::: xpcom/base/nsConsoleService.cpp > @@ +85,5 @@ > > + > > + if (i == mBufferSize - 1) { > > + // If we removed the last item, we just nullify the current slot > > + mMessages[i] = nullptr; > > + mCurrent = mBufferSize - 1; > > I think this is just the degenerate case of the loop below, so you can > handle all the cases with just the loop? You are right. I had hard time to make such simple loop being correct and not crashing in C++ that I didn't realized that! > ::: xpcom/base/nsIConsoleService.idl > @@ +47,5 @@ > > + > > + /** > > + * Clear the message buffer for a given inner window. > > + */ > > + void resetWindow(in uint64_t innerWindowID); > > This is a complete nit, but I think clearMessagesForWindow > (clearMessagesForWindowID?) would be much nicer. I know it's not > symmetrical with |reset()|, but resetWindow() sounds very wrong. I went for clearMessagesForWindowID.

Interdiff on top of attachment 8616785 [details] [diff] [review].

See previous attachment for interdiff. Try run with both patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5b6778d30de (In reply to Bobby Holley (:bholley) from comment #66) > ::: js/src/jsexn.cpp > @@ +325,5 @@ > > > > +JSObject* > > +js::StackFromException(JSContext* cx, HandleObject objArg) > > +{ > > + RootedObject obj(cx, UncheckedUnwrap(objArg)); > > This UncheckedUnwrap seems pretty unsafe to me. Unless there's a good > reason, this should be CheckedUnwrap. > _I have absolutely no idea what is this about._ I just copy pasted ErrorFromException. (See the long comment there) But I'm now using CheckedUnwrap and it works fine too. > ::: js/xpconnect/src/nsScriptErrorWithStack.cpp > @@ +41,5 @@ > > + NS_INTERFACE_MAP_ENTRY(nsIConsoleMessage) > > + NS_INTERFACE_MAP_ENTRY(nsIScriptError) > > +NS_INTERFACE_MAP_END > > + > > +nsScriptErrorWithStack::nsScriptErrorWithStack(JS::HandleValue aStack) > > nsScriptErrorWithStack is only safe if it has a windowID, given the console > leak issue, right? I think it should override ::Init to MOZ_CRASH. Right. I did the override.

Comment on attachment 8629987 [details] [diff] [review] Pass SavedFrame from JS::ErrorObject to nsIScriptError - v8 Review of attachment 8629987 [details] [diff] [review]: ----------------------------------------------------------------- r=bholley with comments addressed and feedback from mccr8. ::: dom/base/nsJSEnvironment.cpp @@ +422,5 @@ > > if (status != nsEventStatus_eConsumeNoDefault) { > + if (mError.isObject()) { > + AutoJSAPI jsapi; > + jsapi.Init(mError.toObjectOrNull()); you need to check the return value of Init() for failure and bail out appropriately. @@ +424,5 @@ > + if (mError.isObject()) { > + AutoJSAPI jsapi; > + jsapi.Init(mError.toObjectOrNull()); > + JSContext* cx = jsapi.cx(); > + JS::RootedObject exObj(cx, mError.toObjectOrNull()); This should be JS::Rooted<JSObject*> outside of SM/XPConnect. @@ +425,5 @@ > + AutoJSAPI jsapi; > + jsapi.Init(mError.toObjectOrNull()); > + JSContext* cx = jsapi.cx(); > + JS::RootedObject exObj(cx, mError.toObjectOrNull()); > + JSAutoCompartment ac(cx, exObj); You don't need this - jsapi.Init() already enters the correct compartment. @@ +426,5 @@ > + jsapi.Init(mError.toObjectOrNull()); > + JSContext* cx = jsapi.cx(); > + JS::RootedObject exObj(cx, mError.toObjectOrNull()); > + JSAutoCompartment ac(cx, exObj); > + JS::RootedObject stackVal(cx, ExceptionStackOrNull(cx, exObj)); given that this is an object, please drop the 'Val'. ::: js/src/vm/SavedStacks.cpp @@ +779,5 @@ > { > THIS_SAVEDFRAME(cx, argc, vp, "(get parent)", args, frame); > RootedObject parent(cx); > (void) JS::GetSavedFrameParent(cx, frame, &parent); > + if (parent && !cx->compartment()->wrap(cx, &parent)) Why is this change necessary? the JSObject* overload of JSCompartment::wrap already null-checks its arg. Please revert this, unless I'm missing something. ::: js/xpconnect/src/nsScriptError.cpp @@ +142,5 @@ > return NS_OK; > } > > NS_IMETHODIMP > +nsScriptError::GetStack(JS::MutableHandleValue aStack) { Do aStack.setUndefined() here. ::: js/xpconnect/src/nsScriptErrorWithStack.cpp @@ +5,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +/* > + * nsScriptErrorWithStack implementation. > + * a main thread only, cycle collected subclass of nsScriptError über-nit: hyphenate "main-thread-only" and "cycle-collected" @@ +6,5 @@ > + > +/* > + * nsScriptErrorWithStack implementation. > + * a main thread only, cycle collected subclass of nsScriptError > + * that allow to track stack trace via SavedFrame object. that can store a SavedFrame stack trace object. @@ +10,5 @@ > + * that allow to track stack trace via SavedFrame object. > + */ > + > +#include "xpcprivate.h" > +#include "jsprf.h" Why this include? @@ +14,5 @@ > +#include "jsprf.h" > +#include "MainThreadUtils.h" > +#include "mozilla/Assertions.h" > +#include "nsGlobalWindow.h" > +#include "nsPIDOMWindow.h" And this? @@ +15,5 @@ > +#include "MainThreadUtils.h" > +#include "mozilla/Assertions.h" > +#include "nsGlobalWindow.h" > +#include "nsPIDOMWindow.h" > +#include "nsILoadContext.h" And this? @@ +16,5 @@ > +#include "mozilla/Assertions.h" > +#include "nsGlobalWindow.h" > +#include "nsPIDOMWindow.h" > +#include "nsILoadContext.h" > +#include "nsIDocShell.h" And this? @@ +21,5 @@ > +#include "nsCycleCollectionParticipant.h" > + > +NS_IMPL_CYCLE_COLLECTION_CLASS(nsScriptErrorWithStack) > + > +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsScriptErrorWithStack) Don't you need to set mStack to undefined here? NI mccr8 to confirm. @@ +70,5 @@ > + AutoJSAPI jsapi; > + jsapi.Init(mStack); > + JSContext* cx = jsapi.cx(); > + JS::RootedValue val(cx, JS::ObjectOrNullValue(mStack)); > + aStack.set(val); You can replace all of this with: aStack.setObjectOrNull(val); which is what your old patch did. Why did you add this? ::: js/xpconnect/src/xpcprivate.h @@ +3036,5 @@ > + NS_DECL_CYCLE_COLLECTING_ISUPPORTS > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(nsScriptErrorWithStack) > + > + NS_IMETHOD > + Init(const nsAString& message, Nit: Put the Init on the same line as NS_IMETHOD

Yes, any field that gets traced needs to be cleared so cycles are broken.

Just rebased due to a conflict.

(In reply to Bobby Holley (:bholley) from comment #71) > ::: js/src/vm/SavedStacks.cpp > @@ +779,5 @@ > > { > > THIS_SAVEDFRAME(cx, argc, vp, "(get parent)", args, frame); > > RootedObject parent(cx); > > (void) JS::GetSavedFrameParent(cx, frame, &parent); > > + if (parent && !cx->compartment()->wrap(cx, &parent)) > > Why is this change necessary? the JSObject* overload of JSCompartment::wrap > already null-checks its arg. Please revert this, unless I'm missing > something. Not necessary anymore, I forgot to pull that off of my patch. > @@ +21,5 @@ > > +#include "nsCycleCollectionParticipant.h" > > + > > +NS_IMPL_CYCLE_COLLECTION_CLASS(nsScriptErrorWithStack) > > + > > +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsScriptErrorWithStack) > > Don't you need to set mStack to undefined here? NI mccr8 to confirm. Ok, done via: tmp->mStack = nullptr; Is that correct? There doesn't seem to be setUndefined() on Heap<JSObject*>. > > @@ +70,5 @@ > > + AutoJSAPI jsapi; > > + jsapi.Init(mStack); > > + JSContext* cx = jsapi.cx(); > > + JS::RootedValue val(cx, JS::ObjectOrNullValue(mStack)); > > + aStack.set(val); > > You can replace all of this with: > > aStack.setObjectOrNull(val); > > which is what your old patch did. Why did you add this? Oh! Much better! I don't know much about the jsapi... I used aStack.set(mStack) before as both were RootedValue. I didn't knew there was setObjectOrNull to set a value from an object.

(In reply to Alexandre Poirot [:ochameau] from comment #74) > > Don't you need to set mStack to undefined here? NI mccr8 to confirm. > > Ok, done via: tmp->mStack = nullptr; > Is that correct? There doesn't seem to be setUndefined() on Heap<JSObject*>. Oh right, sure. I forgot it was a JSObject rather than a JS::Value.

New try before landing the platform patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d95fc540dd1 I'll open a devtool-specific bug for UI tweaks in the webconsole.

Try is still green!

https://hg.mozilla.org/integration/fx-team/rev/3e98575b1f9d https://hg.mozilla.org/integration/fx-team/rev/c5cbaa0e4e4c

https://hg.mozilla.org/mozilla-central/rev/3e98575b1f9d https://hg.mozilla.org/mozilla-central/rev/c5cbaa0e4e4c https://hg.mozilla.org/mozilla-central/rev/83c2b44e598e

Comment on attachment 8631566 [details] [diff] [review] Pass SavedFrame from JS::ErrorObject to nsIScriptError - v9 >+++ b/js/xpconnect/src/xpcprivate.h >+class nsScriptErrorWithStack : public nsScriptError { [..] >+ NS_IMETHOD GetStack(JS::MutableHandleValue); This was missing an 'override' annotation, which causes clang 3.6 & newer to spam a "-Winconsistent-missing-override" build warning, which busts --enable-warnings-as-errors builds (with clang 3.6+). I pushed a followup to add that keyword, with blanket r+ that ehsan granted me for fixes of this sort over in bug 1126447 comment 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/3c908b5d0b15

(In reply to Alexandre Poirot [:ochameau] from comment #18) > This bug is most likely going to fix various existing bugs like: 493414, > 125612, 804772 Are these bugs duplicates now? I wasn't sure for each case.

I went through bugzilla and tried to close/ping/inform all related bugs, clearing the ni?!

Was there a reason to not grab stacks from Exception/DOMException objects here too? Not doing that seems to be leading to bug 1208641.

(In reply to Boris Zbarsky [:bz] (Vacation until Jan 4) from comment #88) > Was there a reason to not grab stacks from Exception/DOMException objects > here too? I really doubt it.