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

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: snorp, Assigned: jchen)

Tracking

unspecified
mozilla32
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

In the background hang monitor (http://darchons.github.io/hang-telemetry-dashboard/bhr.html) 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 http://mozilla.org/MPL/2.0/. */
>  
> +#include "jsfriendapi.h"

Make sure you 'make check-style' on this patch; I think it violates:

https://wiki.mozilla.org/JavaScript:SpiderMonkey:C_Coding_Style#.23include_ordering
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+
https://hg.mozilla.org/mozilla-central/rev/63e728e2fe1c
https://hg.mozilla.org/mozilla-central/rev/d904bd0d1cb5
https://hg.mozilla.org/mozilla-central/rev/627405647ed0
Status: ASSIGNED → RESOLVED
Closed: 5 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.