1387334 - stop exposing startProfiling, stopProfiling etc. and getMaxGCPauseSinceClear, clearMaxGCPauseAccumulator to content

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Frederik Braun [:freddy]]

These functions should not be exposed to content:
> startProfiling
> stopProfiling
> pauseProfilers
> resumeProfilers
> dumpProfile
> getMaxGCPauseSinceClear
> clearMaxGCPauseAccumulator

Bug 1386801 contains some of the discussion around this already, but is mostly about having a test that detects these errors.

Comment 1

[Jason Orendorff [:jorendorff]]

Sorry, we mis-triaged this when it was originally filed.

The fix is to move or remove these:

https://searchfox.org/mozilla-central/source/dom/base/nsJSEnvironment.cpp#1125
https://searchfox.org/mozilla-central/source/dom/workers/RegisterBindings.cpp#38

Bouncing back to DOM.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I'm not sure I follow.

The start/stopProfiling things are intended to be used from performance testcases.  Those are not run as system, hence they must be enabled in non-system globals if we're going to be enabling them at all anywhere.

dumpProfile shouldn't be exposed in non-system globals.  But in the current world, that decision needs to be made inside SpiderMonkey, not DOM.  DOM doesn't have a way to define start/stopProfiling without defining dumpProfile at the moment.

So what exactly is the request here?  Fixing SpiderMonkey so we can define start/stopProfiling without defining dumpProfile?  Removing all this stuff altogether?  Something else?

Comment 3

[Jason Orendorff [:jorendorff]]

This stuff dates back to before e10s and before Gecko profiler. ni?bz to test whether it even still works.

<bz> If not, then this is easy
<bz> We just remove it
<jorendorff> sounds great
<bz> If it does work, feels like we should at least check whether anyone uses it

(Honestly, it sounds to me as though we're going to end up removing it either way.)

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK.  So on Mac, startProfiling/stopProfiling do in fact work (at least on OS 10.12, which is where I tested).  

The first call to startProfiling() prompts for an administrator password to allow dtrace to connect to the process.  So it's not like a web page can just trigger that.

stopProfiling writes out profile data to a caller-supplied directory name under /tmp.  That said, this only works if I manually change the security sandbox level from the default one; in the default configuration the sandbox kills the content process when it tries to do file access.  It does look like this can get hit even if the password was never given for startProfiling (so there isn't actually profile data to write out).

I have to admit, modulo the "have to twiddle the sandbox flags" thing the result is really nice, allowing very targeted profiling, unlike the Gecko profiler.

While I was looking at this stuff, dumpProfile (which was the security worry in bug 1386801) only does something #ifdef MOZ_CALLGRIND which I am pretty sure is not a default configuration.

Comment 5

[Boris Zbarsky [:bzbarsky]]

So one thing we should consider is not defining the various MOZ_PROFILING bits unless compiling with a (non-default!) compile option that would make them non-noops.

Comment 6

[Andrew McCreight [:mccr8]]

JS_DefineProfilingFunctions is called in dom::CreateGlobal, the JS shell, and xpc::InitClassesWithNewWrappedGlobal. Only the first seems to be used for web content. Let's just get rid of the call there. If somebody complains, we can add it back behind an if (false) or something. It seems absurd to keep shipping this without any evidence that anybody actually cares about it.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: Bug 1387334 - Don't call JS_DefineProfilingFunctions in CreateGlobal.

In the modern age of the Gecko Profiler, surely nobody is using this.

Comment 8

[Andrew McCreight [:mccr8]]

FWIW, I accidentally pushed this patch to try.

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Don't call JS_DefineProfilingFunctions in CreateGlobal. r=peterv
https://hg.mozilla.org/integration/autoland/rev/57fad07090a4c5b37531f276b8ca6da5d857969b
https://hg.mozilla.org/mozilla-central/rev/57fad07090a4

Comment 10

[Andrew McCreight [:mccr8]]

I think MOZ_PROFILING is only defined on Nightly, so there's no need to backport this.