1445619 - Convert JS_ShutDown world-leaking warning to an error.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Nicolas B. Pierron [:nbp]]

https://searchfox.org/mozilla-central/rev/8fa0b32c84f924c6809c690117dbd59591f79607/js/src/vm/Initialization.cpp#152

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: It is not longer acceptable to leak the world.

Comment 2

[Andrew McCreight [:mccr8]]

Please don't do this for Firefox builds. This means that all sorts of leaks in Firefox will cause a fatal assert, rather than give a list of leaked objects that might help somebody actually investigate and fix the leak.

Comment 3

[Andrew McCreight [:mccr8]]

If you want tests to fail because of leaked runtimes, the proper way to do that would be to use MOZ_COUNT_CTOR and MOZ_COUNT_DTOR in the JS runtime class.

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8958797 [details] [diff] [review]
It is not longer acceptable to leak the world.

Debug builds do not even build [1], and this is apparently the wrong approach (comment 3).

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e2ebacf476a6b048405f5bc6bc89ffbf5bf3d3e

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: It is not longer acceptable to leak the world.

This is following the recommendation from comment 3, except that we cannot use
the MOZ_COUNT_CTOR as they depend on NS_LogCtor. Thus, I added the JS_COUNT_CTOR
macro, and forwarded the NS_LogCtor function to the JS engine at the
initialization of the process.

So far the leakcheck tests[1] seems to be failing for every parent process. I
will investigate if there is something obvious to fix in this patch or
else-where.

In the mean time I am asking for review on this patch (even if it cannot land as
long as Try is orange), but it seems to me that this patch looks correct.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2e4c3f2fc33c07c62ab60059942329a9dc33df6

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] {backlog: ~36} from comment #5)
> So far the leakcheck tests[1] seems to be failing for every parent process. I
> will investigate if there is something obvious to fix in this patch or
> else-where.
> 
> [1]
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=d2e4c3f2fc33c07c62ab60059942329a9dc33df6

This seems to be an optimized build issue, which apparently do not call the JSRuntime destructor.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8959100 [details] [diff] [review]
It is not longer acceptable to leak the world.

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

Mm.  Kinda fugly, but eh, it works enough.

::: js/src/jsfriendapi.cpp
@@ +1548,5 @@
>      return owner.context() == nullptr;
>  }
> +
> +static mozilla::Atomic<LogCtorDtor> sLogCtor(nullptr);
> +static mozilla::Atomic<LogCtorDtor> sLogDtor(nullptr);

Kinda dubious of these being atomics, certainly doesn't seem necessary unless I'm missing something -- but eh, JSRuntime creation/destruction is not a perf-sensitive thing.  :-)

@@ +1553,5 @@
> +
> +JS_FRIEND_API(void)
> +js::SetLogCtorDtorFunctions(LogCtorDtor ctor, LogCtorDtor dtor)
> +{
> +    sLogCtor = ctor;

MOZ_ASSERT(ctor && dtor);

@@ +1561,5 @@
> +JS_FRIEND_API(void)
> +js::LogCtor(void* self, const char* type, uint32_t sz)
> +{
> +    LogCtorDtor fun = sLogCtor;
> +    if (fun)

if (LogCtorDtor fun = sLogCtor)

@@ +1569,5 @@
> +JS_FRIEND_API(void)
> +js::LogDtor(void* self, const char* type, uint32_t sz)
> +{
> +    LogCtorDtor fun = sLogDtor;
> +    if (fun)

Ditto.

::: js/xpconnect/src/nsXPConnect.cpp
@@ +120,5 @@
>  void
>  nsXPConnect::InitStatics()
>  {
> +    // These functions are used for reporting leaks, we register them as early
> +    // as possible to avoid missing any classes creations.

...leaks, so we register...
...any classes' creations.

Comment 8

[Nicolas B. Pierron [:nbp]]

(In reply to Jeff Walden [:Waldo] from comment #7)
> > +
> > +static mozilla::Atomic<LogCtorDtor> sLogCtor(nullptr);
> > +static mozilla::Atomic<LogCtorDtor> sLogDtor(nullptr);
> 
> Kinda dubious of these being atomics, certainly doesn't seem necessary
> unless I'm missing something -- but eh, JSRuntime creation/destruction is
> not a perf-sensitive thing.  :-)

I guess I've done too much Rust in my life to not consider static without some an Atomic wrapper.

Comment 9

[Andrew McCreight [:mccr8]]

Comment on attachment 8959100 [details] [diff] [review]
It is not longer acceptable to leak the world.

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

Thanks for fixing it to work with our existing leak checking. It'll be interesting to see what it turns up that leak checking on the nsXPConnect object doesn't turn up.

::: js/src/jsfriendapi.cpp
@@ +1551,5 @@
> +static mozilla::Atomic<LogCtorDtor> sLogCtor(nullptr);
> +static mozilla::Atomic<LogCtorDtor> sLogDtor(nullptr);
> +
> +JS_FRIEND_API(void)
> +js::SetLogCtorDtorFunctions(LogCtorDtor ctor, LogCtorDtor dtor)

Like Jeff said, you shouldn't need atomics here, because this method should only be called once. You could assert that the two static variables are null at the start of the function, as a loose check that there's no race. If we're creating more than one XPConnect in a process there's going to be lots more that goes wrong besides this.

::: js/xpconnect/src/nsXPConnect.cpp
@@ +121,5 @@
>  nsXPConnect::InitStatics()
>  {
> +    // These functions are used for reporting leaks, we register them as early
> +    // as possible to avoid missing any classes creations.
> +    js::SetLogCtorDtorFunctions(NS_LogCtor, NS_LogDtor);

That leak you are seeing is funny! I didn't realize we'd even have enough set up to do leak checking in opt builds. Anyways, you just need to guard this with #ifdef NS_BUILD_REFCNT_LOGGING.

Comment 10

[Nicolas B. Pierron [:nbp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=175f4290cb6ea43559e1173aff438727ed766f77

No leak check failures yet, I will cross fingers and push it.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/55b8fe1015cb
It is not longer acceptable to leak the world. r=Waldo,mccr8

Comment 12

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/55b8fe1015cb