1474701 - Shutdown profiles underflow the JSONWriter leading to infinite writing

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement, P1)

Description

[Randell Jesup [:jesup] (needinfo me)]

In saving meta blocks of profiler JSON at shutdown (via MOZ_PROFILER_SHUTDOWN=file), the code unbalances the JSONWriter tree.

Also, there are no safety checks in the JSONWriter code, even in debug builds, against underflow.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Fix shutdown profile-save (fix imbalance) and add JSONWriter assertions

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 8991103 [details] [diff] [review]
Fix shutdown profile-save (fix imbalance) and add JSONWriter assertions

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

::: mfbt/JSONWriter.h
@@ +247,5 @@
>    size_t mDepth;                  // the current nesting depth
>  
>    void Indent()
>    {
> +    MOZ_ASSERT(mDepth >= 0);

`mDepth` is a `size_t`, so this assertion will never fail. No need for it.

@@ +335,3 @@
>      } else {
>        mDepth--;
> +      MOZ_ASSERT(mDepth >= 0);

`mDepth` is a `size_t`, so please move the assertion before the decrement and change it to `mDepth > 0`.

Comment 3

[Markus Stange [:mstange]]

Comment on attachment 8991103 [details] [diff] [review]
Fix shutdown profile-save (fix imbalance) and add JSONWriter assertions

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

What Nick said. Also, yikes. I wonder what kind of API would have prevented this. Probably something RAII-based... but I don't think C++ has the right capabilities to make it impossible to have two overlapping objects at the same time, for example.

Comment 4

[Nicholas Nethercote [inactive]]

> I wonder what kind of API would have prevented this

Yeah: https://searchfox.org/mozilla-central/source/mfbt/JSONWriter.h#23-30... though this bug shows that the last sentence of that comment is over-optimistic :(

Comment 5

[Markus Stange [:mstange]]

Is this ready to land?

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d323d133324b
Fix shutdown profile-save (fix imbalance) and add JSONWriter assertions r=mstange,njn

Comment 7

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/d323d133324b