785656 - Worker dump() implementation should be a no-op by default

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

It currently prints to stdout unconditionally, which isn't desirable in release builds. Other dump() implementations exposed to content (e.g. nsGlobalWindow::Dump) obey browser.dom.window.dump.enabled, which is false by default.

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: dump() should be disabled by default in Workers.

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 814613 [details] [diff] [review]
dump() should be disabled by default in Workers.

Review of attachment 814613 [details] [diff] [review]:
-----------------------------------------------------------------

This is probably harmless but it will at least cause Helgrind/TSan/VTune to issue warnings. We should add a threadsafe function to RuntimeService that will tell us whether this is enabled either 1) using the RuntimeService lock, or 2) just check it at startup and require a restart before a pref change will be honored on workers.

I don't care which we do really since dump() should be pretty rare.

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: dump() should be disabled by default in Workers.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 815375 [details] [diff] [review]
dump() should be disabled by default in Workers.

Review of attachment 815375 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/RuntimeService.cpp
@@ +2213,5 @@
> +RuntimeService::WorkersDumpEnabled()
> +{
> +  MOZ_ASSERT(!NS_IsMainThread());
> +  MutexAutoLock lock(mMutex);
> +  return nsContentUtils::DOMWindowDumpEnabled();

Wrapping a function call in the mutex is not sufficient to avoid the race. You'd have to either a) wrap all the other callers everywhere, or b) make nsContentUtils grab the RuntimeService mutex internally whenever it touches the data. Obviously those are both poor options.

I was talking about making the runtimeservice maintain its own data that we could protect appropriately.

::: dom/workers/WorkerScope.cpp
@@ +588,5 @@
>    static bool
>    Dump(JSContext* aCx, unsigned aArgc, jsval* aVp)
>    {
> +    RuntimeService* runtimeService = RuntimeService::GetService();
> +    if (!runtimeService)

You can just MOZ_ASSERT this.

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: dump() should be disabled by default in Workers.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 816008 [details] [diff] [review]
dump() should be disabled by default in Workers.

Review of attachment 816008 [details] [diff] [review]:
-----------------------------------------------------------------

Hey Nikhil, next time you're in SF come find me and we can talk about the best path forward here.

::: dom/workers/RuntimeService.cpp
@@ +1566,5 @@
> +#if !(defined(DEBUG) || defined(MOZ_ENABLE_JS_DUMP))
> +  {
> +    MutexAutoLock lock(mMutex);
> +    Preferences::AddBoolVarCache(&sWorkersDumpEnabled,
> +                                 "browser.dom.window.dump.enabled");

This lock is a) dangerous, since you're calling out into code you don't control, and b) not protecting anything except for this first write. You're still going to race every time the pref changes.

You're going to have to make a custom pref observer here if you want to support changes during runtime.

::: dom/workers/RuntimeService.h
@@ +103,5 @@
>    static JSSettings sDefaultJSSettings;
>  
> +#if !(defined(DEBUG) || defined(MOZ_ENABLE_JS_DUMP))
> +  // Protected by mMutex.
> +  static bool sWorkersDumpEnabled;

There's no real need for this to be a static member, it can just be a file-level global in RuntimeService.cpp.

::: dom/workers/WorkerScope.cpp
@@ +591,5 @@
> +    RuntimeService* runtimeService = RuntimeService::GetService();
> +    MOZ_ASSERT(runtimeService);
> +
> +    if (!runtimeService->WorkersDumpEnabled())
> +      return true;

Braces, always :)

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Ben, how about, RuntimeService holds a Atomic<uint32_t> gDumpEnabled, and uses a observer function on Preferences in the main thread to modify it, and WorkerScope reads it from the worker thread?

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

My rule of thumb is to always use a lock unless the locking overhead shows up in profiles. Changes to the preference value and calls to dump() are never going to be so frequent. Let's not mess with atomics.

Comment 9

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: dump() should be disabled by default in Workers.

I hope there isn't any stupid code any more.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 818086 [details] [diff] [review]
dump() should be disabled by default in Workers.

Review of attachment 818086 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking pretty close!

::: dom/workers/RuntimeService.cpp
@@ +168,5 @@
>    // XXX Don't care about ProgressEvent since it should never leak to the main
>    // thread.
>  };
>  
> +#if !(defined(DEBUG) || defined(MOZ_ENABLE_JS_DUMP))

How about you only check this once and then #define USING_PREF_DUMP or something? Then check that everywhere.

It will also let you do all your #define work at the top of the file (like for PREF_DOM_WINDOW_DUMP_ENABLED).

@@ +172,5 @@
> +#if !(defined(DEBUG) || defined(MOZ_ENABLE_JS_DUMP))
> +
> +#define PREF_DOM_WINDOW_DUMP_ENABLED "browser.dom.window.dump.enabled"
> +
> +mozilla::Mutex gDumpMutex("RuntimeService gDumpMutex");

This will trigger leak warnings, please just use the existing runtimeservice mutex. You can pass it as the closure property for your pref callback.

@@ +183,5 @@
> +DumpPrefChanged(const char* aPrefName, void* aClosure)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MutexAutoLock lock(gDumpMutex);
> +  gWorkersDumpEnabled = Preferences::GetBool(PREF_DOM_WINDOW_DUMP_ENABLED, false);

Please don't call out to prefs code with the mutex held. Just store the value temporarily on the stack without the mutex held. Then grab the mutex to set gWorkersDumpEnabled.

@@ +192,2 @@
>  static_assert(NS_ARRAY_LENGTH(gStringChars) == ID_COUNT,
>                "gStringChars should have the right length.");

This static assert should stay right next to the gStringChars array.

@@ +1799,5 @@
>          NS_FAILED(Preferences::UnregisterCallback(LoadJSContextOptions,
>                                                    PREF_WORKERS_OPTIONS_PREFIX,
>                                                    nullptr)) ||
> +#if !(defined(DEBUG) || defined(MOZ_ENABLE_JS_DUMP))
> +      NS_FAILED(Preferences::UnregisterCallback(DumpPrefChanged,

Nit: Your indent is off here.

@@ +2243,5 @@
> +bool
> +RuntimeService::WorkersDumpEnabled()
> +{
> +#if !(defined(DEBUG) || defined(MOZ_ENABLE_JS_DUMP))
> +  MOZ_ASSERT(!NS_IsMainThread());

I don't think you need to assert this. Since it's protected by a lock it's safe to be called on any thread.

::: dom/workers/RuntimeService.h
@@ +278,5 @@
>    ScheduleWorker(JSContext* aCx, WorkerPrivate* aWorkerPrivate);
>  
>    static void
>    ShutdownIdleThreads(nsITimer* aTimer, void* aClosure);
> +

Nit: Remove extra newline

::: dom/workers/WorkerScope.cpp
@@ +591,5 @@
> +    RuntimeService* runtimeService = RuntimeService::GetService();
> +    MOZ_ASSERT(runtimeService);
> +
> +    if (!runtimeService->WorkersDumpEnabled())
> +      return true;

Nit: Braces.

Comment 11

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: dump() should be disabled by default in Workers.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 820043 [details] [diff] [review]
dump() should be disabled by default in Workers.

Review of attachment 820043 [details] [diff] [review]:
-----------------------------------------------------------------

Sooooooooo close! Just one thing to fix:

::: dom/workers/RuntimeService.cpp
@@ +1636,5 @@
> +#if DUMP_CONTROLLED_BY_PREF
> +      NS_FAILED(Preferences::RegisterCallbackAndCall(
> +                                              DumpPrefChanged,
> +                                              PREF_DOM_WINDOW_DUMP_ENABLED,
> +                                              static_cast<void*>(&mMutex))) ||

Nit: You shouldn't need to static_cast here. &mMutex should be enough.

@@ +1803,5 @@
>                                                    nullptr)) ||
> +#if DUMP_CONTROLLED_BY_PREF
> +        NS_FAILED(Preferences::UnregisterCallback(DumpPrefChanged,
> +                                                  PREF_DOM_WINDOW_DUMP_ENABLED,
> +                                                  nullptr)) ||

You need to pass the same closure value here or you'll leave the callback in place. Then you'll touch a bad mutex and crash :)

Comment 13

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: dump() should be disabled by default in Workers.

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 824914 [details] [diff] [review]
dump() should be disabled by default in Workers.

Review of attachment 824914 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great! Thanks for doing this :)

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/7dccd60cb553

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7dccd60cb553