1417439 - Release actors when message are pruned due to an MESSAGES_ADD action

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[Nicolas Chevobbe [:nchevobbe]]

In Bug 1371721, we created a new action to batch our message addition into the store. Which means that messages can be pruned when using this action, and implies releasing server actors.
But, at the moment, we only do that when we use the MESSAGE_ADD action.
This wasn't caught by tests because they explicitly use the MESSAGE_ADD action.

In fact, we only use the MESSAGE_ADD action in tests, so it would be better to remove it completely and switch the tests to use MESSAGES_ADD instead. I don't think we should do this as part of this bug because we should uplift the patch for this bug, and we'd better keep the changes as minimal as we can.

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1417439 - Release actors when message are pruned due to an MESSAGES_ADD action; .

In Bug 1371721, we created a new action to batch our message addition into the store. Which means that messages can be pruned when using this action,
and implies releasing server actors.

But, at the moment, we only do that when we use the MESSAGE_ADD action.
This wasn't caught by tests because they explicitly use the MESSAGE_ADD action.

This patch makes the console release actors in reaction to a MESSAGES_ADD
action and add a test to make sure we handle this as expected.

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

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Removal of the messageAdd action happens in Bug 1417446

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8928510 [details]
Bug 1417439 - Release actors when message are pruned due to an MESSAGES_ADD action; .

https://reviewboard.mozilla.org/r/199764/#review206376

Looks good, just couple of inline comments.

R+ assuming try is green

Honza

::: devtools/client/webconsole/new-console-output/test/helpers.js:28
(Diff revision 1)
> -
>    const idGenerator = new IdGenerator();
> -  wrappedActions.messageAdd = (packet) => {
> -    return actions.messageAdd(packet, idGenerator);
> -  };
> -
> +  return Object.assign({}, actions, {
> +    messageAdd: packet => actions.messageAdd(packet, idGenerator),
> +    messagesAdd: packets => actions.messagesAdd(packets, idGenerator)
> +  });

It would be nice to start using spread operator instead of Object.assign() like as follows:

return {
  ...actions,
  messageAdd: packet => actions.messageAdd(packet, idGenerator),
  messagesAdd: packets => actions.messagesAdd(packets, idGenerator),
}

In order to avoid IDE eslint warnings you can create eslintrc.js (in webconsole dir):

module.exports = {
  "env": {
    "browser": true,
  },

  "extends": "../../.eslintrc.js",

  "parserOptions": {
    "ecmaFeatures": {
      experimentalObjectRestSpread: true,
    },
  },
};

::: devtools/client/webconsole/new-console-output/test/store/release-actors.test.js:66
(Diff revision 1)
>        expect(releasedActors).toInclude(firstMessageActor);
>        expect(releasedActors).toInclude(secondMessageActor);
>        expect(releasedActors).toInclude(thirdMessageActor);
>      });
>  
> +    it("releases backend actors when limit reached adding multiples messages", () => {

typo: adding multiples => adding multiple

Comment 4

[Mike Taylor [:miketaylr]]

Nicolas, is this important to get landed and uplifted for 58?

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

It is (otherwise the console is leaking).  I forgot I got review on this, so let me land it and request uplift

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8928510 [details]
Bug 1417439 - Release actors when message are pruned due to an MESSAGES_ADD action; .

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

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8928510 [details]
Bug 1417439 - Release actors when message are pruned due to an MESSAGES_ADD action; .

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8d93ad26023
Release actors when message are pruned due to an MESSAGES_ADD action; r=Honza.

Comment 9

[Bogdan Tara[:bogdan_tara | bogdant]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Does this have a user impact that justifies uplift to Beta or can it ride the 59 train?

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

It does have a user impact and I'd like it to be uplifted (it's a narrowed patch, well tested)

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8928510 [details]
Bug 1417439 - Release actors when message are pruned due to an MESSAGES_ADD action; .

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1371721
[User impact if declined]: The console will leak, causing firefox to eat too much memory on long debugging sessions
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: -
[List of other uplifts needed for the feature/fix]: -
[Is the change risky?]: No
[Why is the change risky/not risky?]: Very narrowed and well tested
[String changes made/needed]: -

Comment 13

[Gerry Chang [:gchang]]

Comment on attachment 8928510 [details]
Bug 1417439 - Release actors when message are pruned due to an MESSAGES_ADD action; .

Fix a potential memory leak issue on long debugging session. Beta58+.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/35df0c984d4a