866231 - remote protocol profiler actors struggle with cyclic data in eventNotification packets

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Jim Blandy :jimb]

The remote protocol actor representing the Gecko profiler shares observer notifications with the client in a problematic way:

https://hg.mozilla.org/mozilla-central/file/d360244c69ab/toolkit/devtools/debugger/server/dbg-profiler-actors.js#l102

Because notification subjects can be cyclic, the actor carefully breaks the cycle, transmits the packet, and then restores the cycle. This is okay if one can trust the subject to be generally something that can b transmitted meaningfully, once the cycles are broken.

But that strategy doesn't play well with this:

https://hg.mozilla.org/mozilla-central/file/d360244c69ab/toolkit/devtools/debugger/dbg-transport.js#l223

Here, we actually deliver the message on a separate event tick. So by the time the recipient gets it, the cycle is back.

Although it has a bug at the moment, we're supposed to be freezing packets that go through LocalDebuggerTransport; once that's fixed, the cycle restoration won't work any more. This also means that trying to log the packets when they are processed, instead of when they're delivered, attempts to JSONify the packet when it is cyclic again.

Right now, it sort of works because DebuggerTransport (that actually uses a stream, and is cycle-sensitive) *does* send immediately; and although LocalDebuggerTransport doesn't send immediately, it doesn't use a stream (unless one is logging) and is thus not cycle-sensitive.

Comment 1

[Jim Blandy :jimb]

Attachment: Have the profiler actor wrap notifications in an extra level of JSON, so that we can use a replacer function to cope with cyclic structures.

Comment 2

[Jim Blandy :jimb]

Attachment: Handle extra level of JSONification in profiler plugin

This is untested, but it's what I would expect to be needed to have the profiler plugin cope with the prior patch. BenWa, does this look okay?

Comment 3

[Jim Blandy :jimb]

https://tbpl.mozilla.org/?tree=Try&rev=f567d9afa40c

Comment 4

[Dave Camp (:dcamp)]

Comment on attachment 742498 [details] [diff] [review]
Have the profiler actor wrap notifications in an extra level of JSON, so that we can use a replacer function to cope with cyclic structures.

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

::: toolkit/devtools/debugger/server/dbg-profiler-actors.js
@@ +121,5 @@
>      this.conn.send({ from: this.actorID,
>                       type: "eventNotification",
>                       event: aTopic,
> +                     subjectJSON: JSON.stringify(aSubject.wrappedJSObject, cycleBreaker),
> +                     dataJSON: JSON.stringify(aData, cycleBreaker) });

Unless I'm missing something, this seems like it makes the protocol weirder to make implementation easier?  We don't pass stringified json anywhere else do we?

Could we just "subject: JSON.parse(JSON.stringify(aSubject.wrappedJSObject, cycleBreaker))" at the expense of some potential double-stringifying when not using the local transport?

::: toolkit/devtools/debugger/tests/unit/test_profiler_actor.js
@@ +73,5 @@
>          do_check_eq(aType, "eventNotification");
>          do_check_eq(aData.event, "foo");
> +        do_check_eq(typeof aData.subjectJSON, "string");
> +        do_check_eq(aData.subjectJSON, '{"foo":"foo"}');
> +        do_check_eq(aData.dataJSON, '"foo"');

If we do decide to make a protocol change, I imagine there's frontend making use of this data that we should be updating too?

Comment 5

[Jim Blandy :jimb]

(In reply to Dave Camp (:dcamp) from comment #4)
> Comment on attachment 742498 [details] [diff] [review]
> Have the profiler actor wrap notifications in an extra level of JSON, so
> that we can use a replacer function to cope with cyclic structures.
> 
> Review of attachment 742498 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/debugger/server/dbg-profiler-actors.js
> @@ +121,5 @@
> >      this.conn.send({ from: this.actorID,
> >                       type: "eventNotification",
> >                       event: aTopic,
> > +                     subjectJSON: JSON.stringify(aSubject.wrappedJSObject, cycleBreaker),
> > +                     dataJSON: JSON.stringify(aData, cycleBreaker) });
> 
> Unless I'm missing something, this seems like it makes the protocol weirder
> to make implementation easier?

Well, yes. This was BenWa's suggestion, so I didn't think much about it.

> We don't pass stringified json anywhere else do we?

The debugger doesn't, but the profiler does. For example, the "getProfile" request has a reply of the form { profile: { libs: DATA, ... } } where DATA is a string of JSON. (I don't know why.)

> Could we just "subject: JSON.parse(JSON.stringify(aSubject.wrappedJSObject,
> cycleBreaker))" at the expense of some potential double-stringifying when
> not using the local transport?

Sure, that would work.

> ::: toolkit/devtools/debugger/tests/unit/test_profiler_actor.js
> @@ +73,5 @@
> >          do_check_eq(aType, "eventNotification");
> >          do_check_eq(aData.event, "foo");
> > +        do_check_eq(typeof aData.subjectJSON, "string");
> > +        do_check_eq(aData.subjectJSON, '{"foo":"foo"}');
> > +        do_check_eq(aData.dataJSON, '"foo"');
> 
> If we do decide to make a protocol change, I imagine there's frontend making
> use of this data that we should be updating too?

Yes, that's the other patch in this bug - for the profiler plugin, which is not in the tree. BenWa said it looked good.

Comment 6

[Jim Blandy :jimb]

Attachment: Have the remote protocol profiler actor copy notification values and break cycles before sending them to the client.

This implements the review's suggestions; no change to the protocol.

Comment 7

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f85b44d7819

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3f85b44d7819