1408321 - Actors of EvaluationResult messages are not released

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[Nicolas Chevobbe [:nchevobbe]]

Steps to reproduce:
1. Open the console
2. Evaluate `({a: [1]})`
3. Open the browser toolbox debugger and add a breakpoint on http://searchfox.org/mozilla-central/rev/ed1d5223adcdc07e9a2589ee20f4e5ee91b4df10/devtools/client/webconsole/new-console-output/store.js#217
4. Click the "clear console" button
5. The debugger should pause
6. Inspect the `removedActors` variable

Expected results:
the array contains one actor

Actual results:
the array is empty, and the actor is never released on the server (until the toolbox gets destroyed)

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1408321 - Change EvaluationResult message shape to match other messages;.

The parameters property in EvaluationResult message was a plain object,
whereas in other message types, it is an array. This could cause some issues
in the reducer, in particular for releasing actors.
Changing the parameters from object to array gives us a consistent way
of dealing with this property through the console app.
The change required the stubs to be updated, as well as a minor change in the
EvaluationResult component.
A test was added to make sure we do release actors from EvaluationResult messages.

Review commit: https://reviewboard.mozilla.org/r/189102/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/189102/

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8918226 [details]
Bug 1408321 - Change EvaluationResult message shape to match other messages;.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189102/diff/1-2/

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 8918226 [details]
Bug 1408321 - Change EvaluationResult message shape to match other messages;.

https://reviewboard.mozilla.org/r/189102/#review194438

::: devtools/client/webconsole/new-console-output/components/message-types/EvaluationResult.js:53
(Diff revision 2)
>    let messageBody;
>    if (message.messageText) {
>      if (typeof message.messageText === "string") {
>        messageBody = message.messageText;
>      } else if (
>        typeof message.messageText === "object"

Do we need to deal with string / longstring results in a similar manner?

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

So, I do think we need to keep the messageText property, since it's used in every component, but yes, we should release actors if messageText is a longstring, which I think we don't do for now.

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Let's handle longStrings in Bug 1408845

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5e8fe51d3a1c
Change EvaluationResult message shape to match other messages;r=bgrins.

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/5e8fe51d3a1c