Closed Bug 1417439 Opened 2 years ago Closed 2 years ago

Release actors when message are pruned due to an MESSAGES_ADD action

Categories

(DevTools :: Console, defect, P1)

defect

Tracking

(firefox-esr52 unaffected, firefox57 unaffected, firefox58 fixed, firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 --- fixed
firefox59 --- fixed

People

(Reporter: nchevobbe, Assigned: nchevobbe)

References

Details

(Keywords: regression)

Attachments

(1 file)

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.
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Has Regression Range: --- → yes
Keywords: regression
Priority: -- → P1
Depends on: 1371721
Blocks: 1417446
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
Attachment #8928510 - Flags: review?(odvarko) → review+
Nicolas, is this important to get landed and uplifted for 58?
Flags: needinfo?(nchevobbe)
It is (otherwise the console is leaking).  I forgot I got review on this, so let me land it and request uplift
Flags: needinfo?(nchevobbe)
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.
https://hg.mozilla.org/mozilla-central/rev/b8d93ad26023
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Does this have a user impact that justifies uplift to Beta or can it ride the 59 train?
Flags: needinfo?(nchevobbe)
It does have a user impact and I'd like it to be uplifted (it's a narrowed patch, well tested)
Flags: needinfo?(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]: -
Attachment #8928510 - Flags: approval-mozilla-beta?
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+.
Attachment #8928510 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.