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

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

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.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

It would be nice to have the script name, too, at least for chrome hangs.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Change js::RunScript to a JS pseudostack entry (v1)

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.

Comment 3

[Benoit Girard (:BenWa)]

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.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

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.

Comment 5

[Kannan Vijayan [:djvj]]

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.

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Move script principals APIs to jsfriendapi (v1)

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?

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Distinguish chrome and content scripts in pseudostack (v1)

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.

Comment 8

[Luke Wagner [:luke]]

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

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

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.

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Distinguish chrome and content scripts in pseudostack (v1.1)

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/.

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/63e728e2fe1c
https://hg.mozilla.org/integration/mozilla-inbound/rev/d904bd0d1cb5
https://hg.mozilla.org/integration/mozilla-inbound/rev/627405647ed0

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/63e728e2fe1c
https://hg.mozilla.org/mozilla-central/rev/d904bd0d1cb5
https://hg.mozilla.org/mozilla-central/rev/627405647ed0