Closed Bug 1013326 Opened 7 years ago Closed 7 years ago

Add a way to distinguish chrome hangs from content ones in BHR


(Core :: XPCOM, defect)

Not set





(Reporter: snorp, Assigned: jchen)




(3 files, 1 obsolete file)

In the background hang monitor ( you often see RunScript() reported. This can either be content JS or chrome. It would be nice if we had some way of distinguishing those here. Maybe just a fake PseudoStack entry would suffice.
It would be nice to have the script name, too, at least for chrome hangs.
BenWa, do you see any problems with switching the js::RunScript frame from a C++ one to a JS one? This will help the hang monitor detect whether a chrome script or a content script was last run. We often don't have more JS frames when a hang happens because the profiler is not running, so the js::RunScript frame is likely our only clue.
Attachment #8432614 - Flags: review?(bgirard)
Comment on attachment 8432614 [details] [diff] [review]
Change js::RunScript to a JS pseudostack entry (v1)

Review of attachment 8432614 [details] [diff] [review]:

Looks fine but you should get a review from the JS team who owns this code.
Attachment #8432614 - Flags: review?(bgirard) → feedback+
Comment on attachment 8432614 [details] [diff] [review]
Change js::RunScript to a JS pseudostack entry (v1)

Review of attachment 8432614 [details] [diff] [review]:

See comment 2.
Attachment #8432614 - Flags: review?(kvijayan)
Comment on attachment 8432614 [details] [diff] [review]
Change js::RunScript to a JS pseudostack entry (v1)

Review of attachment 8432614 [details] [diff] [review]:

::: js/src/vm/SPSProfiler.cpp
@@ +323,5 @@
>          profiler = nullptr;
>          return;
>      }
>      size_before = *profiler->size_;
> +    profiler->push("js::RunScript", nullptr, script, script->code(), /* copy = */ false);

I'm a bit concerned that this entry will look to the profiler like a pushed JS frame of a javascript function named "js::RunScript".  But if Benoit doesn't think this is a big deal, it seems ok.
Attachment #8432614 - Flags: review?(kvijayan) → review+
One of my patches needs to use JS_GetScriptPrincipals. It's in OldDebugAPI.h, and I'm not sure if I should move it to jsfriendapi.h (or another place) first?
Attachment #8434387 - Flags: review?(luke)
This patch checks whether a pseudostack entry is a JS frame, and records whether the JS script is a chrome script. Support for file names will be added in followup bugs.
Attachment #8434398 - Flags: review?(snorp)
Comment on attachment 8434387 [details] [diff] [review]
Move script principals APIs to jsfriendapi (v1)

Review of attachment 8434387 [details] [diff] [review]:

::: js/src/jsapi-tests/testCloneScript.cpp
@@ +6,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at */
> +#include "jsfriendapi.h"

Make sure you 'make check-style' on this patch; I think it violates:
Attachment #8434387 - Flags: review?(luke) → review+
Comment on attachment 8434398 [details] [diff] [review]
Distinguish chrome and content scripts in pseudostack (v1)

Review of attachment 8434398 [details] [diff] [review]:

::: xpcom/threads/ThreadStackHelper.cpp
@@ +218,5 @@
> +bool
> +IsChromeJSScript(JSScript* aScript)
> +{
> +  // May be called from another thread or inside a signal handler.
> +  // We assume querying the script is safe but we must not manipulate it.

This is Super Scary. If JS people claim it's ok, though, then I guess that's good enough for me.
Attachment #8434398 - Flags: review?(snorp) → review+
I had to switch to using nsScriptSecurityManager instead of nsContentUtils because of a namespace collision problem with unified builds. The functionality is the same, and I think it makes more sense this way because I'm not sure xpcom/ should be including stuff from content/.
Attachment #8434398 - Attachment is obsolete: true
Attachment #8435893 - Flags: review+
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1028775
You need to log in before you can comment on or make changes to this bug.