Bug 1050500 requires simple labels explaining why a given piece of JavaScript code was invoked: "requestAnimationFrame" or "scriptLoader", for example. In bug 961325, bug 971673, and elsewhere, we have designed elaborate mechanisms by which Gecko could provide all sorts of detailed information about these invocations, nicely connected with other debugging improvements (like correctly setting the entry global). But bug 1050500 is preventing important developer tools from being shipped, and it doesn't need all that. It can be addressed without impeding those other less urgent fixes. The AutoEntryScript constructor should take a simple static C string as an argument, and convey it to nsDocShell::NotifyJSRunToCompletionStart, to be included in the markers consumed by the perf tools. WebIDL Functions are reflected in C++ as objects with a "Call" method. Since "WebIDL callback" is not a very informative execution reason, Call should accept a reason as well, to be passed through to its AutoEntryScript. We should resuscitate Boris's last patch to add execution reasons to Call (attachment 8424405 [details] [diff] [review]). And, we should simplify and rebase Panos's last patch adding execution reason strings to all call sites (attachment 8433491 [details] [diff] [review]).

Here's a patch adding the constructor argument to AutoEntryScript. This makes the argument optional to avoid breaking the entire tree, so Boris can use it as the basis for resuscitating his Call patch, but we'll make it required before landing.

This is updated to tip and just uses const char* as the execution reason. Callback invocations are not forced to pass in an execution reason. If they do not, the name of the thing being invoked will be used (e.g. "Function" or "EventListener.handleEvent" or "PromiseInit").

Tom, can you give this a try?

(In reply to Jim Blandy :jimb from comment #3) > Created attachment 8591091 [details] [diff] [review] > Add 'aReason' argument to AutoEntryScript constructor, and provide plausible > names for its instantiations. > > Tom, can you give this a try? This seems to work fine for me, thanks. I tested it by adding the new data to the timeline markers.

Comment on attachment 8591091 [details] [diff] [review] Add 'aReason' argument to AutoEntryScript constructor, and provide plausible names for its instantiations. Review of attachment 8591091 [details] [diff] [review]: ----------------------------------------------------------------- Flagging Peter to look at the codegen. r=me on everything else I guess, though I wish we had a more principled naming scheme. ::: docshell/base/nsIDocShell.idl @@ +1038,5 @@ > * Notify DocShell when the browser is about to start executing JS, and after > * that execution has stopped. This only occurs when the Timeline devtool > * is collecting information. > */ > + [noscript,notxpcom,nostdcall] void notifyJSRunToCompletionStart(in string aReason); Please rev the iid, just in case. ::: dom/base/nsFrameMessageManager.cpp @@ +1061,5 @@ > // } > // JSContext* cx = aes.cx(); > nsIGlobalObject* nativeGlobal = > xpc::NativeGlobal(js::GetGlobalForObjectCrossCompartment(wrappedJS->GetJSObject())); > + AutoEntryScript aes(nativeGlobal, "nsIFrameMessageManager receiveMessage"); "message manager handler" @@ +1566,5 @@ > > JS::Rooted<JSObject*> global(rt, mGlobal->GetJSObject()); > if (global) { > + AutoEntryScript aes(xpc::NativeGlobal(global), > + "nsIMessageManager load script"); "message manager script load" ::: dom/base/nsScriptLoader.cpp @@ +1120,5 @@ > } > > // New script entry point required, due to the "Create a script" sub-step of > // http://www.whatwg.org/specs/web-apps/current-work/#execute-the-script-block > + AutoEntryScript entryScript(globalObject, "scriptLoader", true, "<script> element" ::: dom/base/nsTraversal.cpp @@ +61,5 @@ > if (mFilter.HasWebIDLCallback()) { > AutoRestore<bool> inAcceptNode(mInAcceptNode); > mInAcceptNode = true; > + // No need to pass in an execution reason, since NodeFilter.acceptNode > + // is pretty much exactly what we'd say anyway. Why add this comment here and not in all of the dozens of other places where we invoke a WebIDL callback? ::: dom/promise/Promise.cpp @@ +217,5 @@ > > JS::Rooted<JSObject*> rootedThenable(cx, mThenable); > > mThen->Call(rootedThenable, resolveFunc, rejectFunc, rv, > + "DOM Promise thenable", CallbackObject::eRethrowExceptions, "promise thenable" @@ +610,5 @@ > aRv.Throw(NS_ERROR_UNEXPECTED); > return; > } > > + aInit.Call(resolveFunc, rejectFunc, aRv, "DOM Promise executor", "promise initializer" ::: dom/promise/PromiseCallback.cpp @@ +204,5 @@ > ErrorResult rv; > > // PromiseReactionTask step 6 > JS::Rooted<JS::Value> retValue(aCx); > + mCallback->Call(value, &retValue, rv, "DOM promise handler", "promise callback" ::: dom/workers/WorkerRunnable.cpp @@ +331,5 @@ > mozilla::dom::AutoJSAPI jsapi; > Maybe<mozilla::dom::AutoEntryScript> aes; > JSContext* cx; > if (globalObject) { > + aes.emplace(globalObject, "Worker runnable", It seems like most other places the strings are not capitalized. Please be consistent about that. @@ +352,5 @@ > > // In the case of CompileScriptRunnnable, WorkerRun above can cause us to > // lazily create a global, so we construct aes here before calling PostRun. > if (targetIsWorkerThread && !aes && DefaultGlobalObject()) { > + aes.emplace(DefaultGlobalObject(), "Worker PostRun", Just call this Worker runnable - it's the same aes, we're just trying a second time to lazily instantiate it. ::: dom/xbl/nsXBLProtoImplField.cpp @@ +401,5 @@ > } > > // We are going to run script via EvaluateString, so we need a script entry > // point, but as this is XBL related it does not appear in the HTML spec. > + AutoEntryScript entryScript(globalObject, "XBL field", true); XBL <field> initialization ::: dom/xbl/nsXBLProtoImplMethod.cpp @@ +293,5 @@ > } > > // We are going to run script via JS::Call, so we need a script entry point, > // but as this is XBL related it does not appear in the HTML spec. > + dom::AutoEntryScript aes(global, "XBL method"); XBL <constructor>/<destructor> invocation ::: dom/xul/XULDocument.cpp @@ +3563,5 @@ > > // Execute the precompiled script with the given version. > // We're about to run script via JS::CloneAndExecuteScript, so we need an > // AutoEntryScript. This is Gecko specific and not in any spec. > + AutoEntryScript aes(mScriptGlobalObject, "XUL script"); "precompiled XUL <script> element" ::: js/xpconnect/loader/mozJSComponentLoader.cpp @@ +617,5 @@ > if (createdNewGlobal) { > // AutoEntryScript required to invoke debugger hook, which is a > // Gecko-specific concept at present. > + dom::AutoEntryScript aes(NativeGlobal(holder->GetJSObject()), > + "@mozilla.org/moz/jsloader;1 report global"); I'd just say "component loader" rather than giving the contractid, here and below. ::: js/xpconnect/src/Sandbox.cpp @@ +1521,5 @@ > bool ok = true; > { > // We're about to evaluate script, so make an AutoEntryScript. > // This is clearly Gecko-specific and not in any spec. > + mozilla::dom::AutoEntryScript aes(priv, "xpc::EvalInSandbox"); XPConnect sandbox evaluation"

(In reply to Bobby Holley (:bholley) from comment #5) > Flagging Peter to look at the codegen. r=me on everything else I guess, > though I wish we had a more principled naming scheme. Strongly agree; I really wasn't sure what names to attach to most of these spots. Thanks for stepping up. > > + // No need to pass in an execution reason, since NodeFilter.acceptNode > > + // is pretty much exactly what we'd say anyway. > > Why add this comment here and not in all of the dozens of other places where > we invoke a WebIDL callback? I'm not sure. This is from bz's patch to the code generator; perhaps it was meant more for my benefit. I'll take it out. > It seems like most other places the strings are not capitalized. Please be > consistent about that. I was trying to name things after identifiers that would actually appear in the code, so that people could relate the label to what they'd written. Hence my use of "nsIMessageManager" (the interface), "Promise" (the constructor), and "Worker" (the constructor). (You have a better overview of the code so I'm happy to use the names you suggest, but I wasn't being careless, except for the uncapitalized "promise".) > @@ +352,5 @@ > > > > // In the case of CompileScriptRunnnable, WorkerRun above can cause us to > > // lazily create a global, so we construct aes here before calling PostRun. > > if (targetIsWorkerThread && !aes && DefaultGlobalObject()) { > > + aes.emplace(DefaultGlobalObject(), "Worker PostRun", > > Just call this Worker runnable - it's the same aes, we're just trying a > second time to lazily instantiate it. Thanks. I couldn't figure out what this code was doing; it's not clear which JS invocations that aes is meant to protect.

> This is from bz's patch to the code generator; I added the comment there because this is a callsite that explicitly passes null instead of just relying on the default arg value (because it has to pass a non-default value for the next arg). Given that, it seemed like documenting why explicitly passing null is OK was desirable... In the other places, the fact that we're relying on default behavior is not as in-your-face, and in some of those we _should_ perhaps add better descriptive strings in fact.

The problem with naming the indicators by the code they're in is that it creates an external dependency on method names and the like. I think people can just grep for the string (so we should make them sufficiently unique).

Comment on attachment 8591091 [details] [diff] [review] Add 'aReason' argument to AutoEntryScript constructor, and provide plausible names for its instantiations. Review of attachment 8591091 [details] [diff] [review]: ----------------------------------------------------------------- rs=me on the Codegen.py piece since apparently bz wrote the patch, which is good enough for me.

Comment on attachment 8591091 [details] [diff] [review] Add 'aReason' argument to AutoEntryScript constructor, and provide plausible names for its instantiations. I'd still appreciate Peter giving the codegen bits here a once-over when he gets the chance.