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.

Removal of the messageAdd action happens in Bug 1417446

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

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

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

Pushed by nchevobbe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b8d93ad26023 Release actors when message are pruned due to an MESSAGES_ADD action; r=Honza.

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

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

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 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+.