Closed Bug 1307884 Opened 8 years ago Closed 8 years ago

[Investigate] Avoid memory issues by clearing out old messages from store

Categories

(DevTools :: Console, enhancement, P1)

Product:
DevTools
Component:
Console
Type:
enhancement

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Iteration:
55.6 - May 29
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: linclark, Assigned: Honza)

References

Details

(Whiteboard: [console-html])

Attachments

(4 files)

Bug 1307884 - Limit for messages in the store;
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1307884 - Clean up actors;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details
Bug 1307884 - Remove prune function;
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1307884 - Implement tests;
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Originally posted by:linclark see https://github.com/devtools-html/gecko-dev/issues/133 We discussed this during the implementation of #45. We have a message limit which limits how many messages are displayed. However, we keep them in the store even if we're past the limit. It's not clear at what point this will become a problem for our users (or whether it will). If we decide to address it, [this issue](https://github.com/reactjs/redux/issues/644) has some discussion.
Priority: -- → P2
Whiteboard: new-console
Needs investigation to see if this is still an issue, especially with Bug 1308216
Summary: Avoid memory issues by clearing out old messages from store → [Investigate] Avoid memory issues by clearing out old messages from store
Blocks: enable-new-console
Flags: qe-verify-
Whiteboard: new-console → [new-console]
Priority: P2 → P1
Priority: P1 → P2
Whiteboard: [new-console] → [console-html]
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Can you also check into if we need to be calling proxy.releaseActor if there are object actors in the messages? The old frontend is doing this in destroyItem https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/webconsole.js#2283 and https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/webconsole.js#2776
Iteration: --- → 55.5 - May 15
Priority: P2 → P1
First version of the patch attached. Tests will come in separate patch. I'll also take a look at the actor clean up. Honza
Comment on attachment 8864869 [details] Bug 1307884 - Limit for messages in the store; https://reviewboard.mozilla.org/r/136554/#review139670 Just some initial feedback ::: devtools/client/webconsole/new-console-output/reducers/messages.js:94 (Diff revision 1) > + // Remove top level message if the total count of top level messages > + // exceeds the current limit. > + let topLevelCount = getToplevelMessageCount(record); > + while (topLevelCount > logLimit) { > + let removedMessage = removeFirstMessage(record); > + if (!removedMessage.groupId) { Is there a scenario here where the last message to be removed is a group parent, we don't re-enter this loop after removing it, and end up with orphaned group children? Maybe removeFirstMessage should handle the case where the message is a group parent and then also remove all group children, and then this loop doesn't have to even check removedMessage.groupId. ::: devtools/client/webconsole/new-console-output/reducers/messages.js:158 (Diff revision 1) > +/** > + * Returns total count of top level messages (those which are not within a group). > + */ > +function getToplevelMessageCount(record) { > + let count = 0; > + record.messagesById.forEach(message => { This could be shrunk to something like `return messagesById.filter(message=>!message.groupID).length`
Attachment #8864869 - Flags: review?(bgrinstead)
Comment on attachment 8864869 [details] Bug 1307884 - Limit for messages in the store; https://reviewboard.mozilla.org/r/136554/#review140660 We can remove the logLimit / prune logic from the message selector with this change, right? ::: devtools/client/webconsole/new-console-output/reducers/messages.js:158 (Diff revision 2) > +/** > + * Returns total count of top level messages (those which are not > + * within a group). > + */ > +function getToplevelMessageCount(record) { > + return[...record.messagesById].filter(message => !message.groupId).length; Missing space
Attachment #8864869 - Flags: review?(bgrinstead)
I believe we should also be calling proxy.releaseActor for any object actors in the messages on their way out https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/webconsole.js#2778
@bgrins: yes, see the second attached patch. @ochameau: Please see, the second patch. It's the thing that I mentioned on IRC. I am not sure how we could call `releaseMany`, how could I access thread client method that supports that? Honza
Flags: needinfo?(poirot.alex)
Comment on attachment 8865992 [details] Bug 1307884 - Clean up actors; https://reviewboard.mozilla.org/r/137590/#review140758 ::: devtools/client/webconsole/new-console-output/components/message-container.js:67 (Diff revision 1) > || responseChanged > || totalTimeChanged > || timestampVisibleChanged; > }, > > + componentWillUnmount() { I don't think we want to do this based on the component unmounting, since it could umount due to being filtered out, and we'd need to re-render it later. I believe this should instead happen when: 1) a message is removed from the store 2) the frontend is destroyed (I guess all messages in the store should release actors in this case - I'd like Alex to confirm if he could)
Attachment #8865992 - Flags: review?(bgrinstead) → review-
Comment on attachment 8864869 [details] Bug 1307884 - Limit for messages in the store; Requesting feedback from Nicolas since he is looking into some adjacent changes in the selector/reducer
Attachment #8864869 - Flags: feedback?(nchevobbe)
(In reply to Jan Honza Odvarko [:Honza] from comment #11) > @ochameau: Please see, the second patch. It's the thing that I mentioned on > IRC. I am not sure how we could call `releaseMany`, how could I access > thread client method that supports that? releaseMany is just for the thread actor, it is not meant to be used for the console. If you need a bulk release method, you should be implementing your own. It may actually be possible to have such generic method on the root actor. release: function (actorIDs) { for (let actorID of actorsIDs) { // Fetch the ActorPool for this actor let pool = this.conn.getActor(actorID); if (pool) { let actor = pool.get(actorID); // Removing the actor from the pool is going to call its `destroy` method pool.removeActor(actor); } } } Note that you would have to introduce destroy method on actors, calling their existing release method. I don't know why, but today we only do that for LonStringActor... http://searchfox.org/mozilla-central/source/devtools/server/actors/object.js#2091 (These are very old actors that pre-dates protocol.js and many other evolutions on the server side) In your case you should track all the actors created by the webconsole. To do that, follow the usages of this ActorPool: http://searchfox.org/mozilla-central/source/devtools/server/actors/webconsole.js#58 - createEnvironmentActor should be your interest as it sounds related to jsterm - getNetworkEventActor should be watched if you display network events - createValueGrip is your main scenario, where all the grip actors are created. You should ensure that for any actor added to the pool, you end up destroying them once they go out of your view limits. (In reply to Brian Grinstead [:bgrins] from comment #12) > 2) the frontend is destroyed (I guess all messages in the store should > release actors in this case - I'd like Alex to confirm if he could) If you disconnect the client, yes, everything will be destroyed. Otherwise I don't see anything else today that do that explicitely. The easiest thing is to close the DebuggerClient. It will cleanup everything on its own. It happens when you close the toolbox.
Flags: needinfo?(poirot.alex)
Comment on attachment 8864869 [details] Bug 1307884 - Limit for messages in the store; https://reviewboard.mozilla.org/r/136554/#review140982 ::: devtools/client/webconsole/new-console-output/reducers/messages.js:158 (Diff revision 3) > +/** > + * Returns total count of top level messages (those which are not > + * within a group). > + */ > +function getToplevelMessageCount(record) { > + return [...record.messagesById].filter(message => !message.groupId).length; this is interesting, I didn't thought we could spread an immutable list, looks great ::: devtools/client/webconsole/new-console-output/reducers/messages.js:169 (Diff revision 3) > + // Remove from list of opened groups. > + let uiIndex = record.messagesUiById.indexOf(firstMessage); > + if (uiIndex >= 0) { > + record.set("messagesUiById", record.messagesUiById.delete(uiIndex)); > + } > + > + // Remove from list of tables. > + if (record.messagesTableDataById.has(firstMessage.id)) { > + record.set("messagesTableDataById", record.messagesTableDataById.delete(firstMessage.id)); > + } > + > + // Remove from list of parent groups. > + if (record.groupsById.has(firstMessage.id)) { > + record.set("groupsById", record.groupsById.delete(firstMessage.id)); > + } I'm a bit worried this could be costly with the recursive call happening in l.191 Do you think this cleanup can be done in the message add switch block ? We wouldh ave to collect all the removed message and then have a cleanup function call for all of them, which would to the work in a withMutations callback. I don't know if it's doable easily or if it will be better in the end that what we're doing here, but I think we can try.
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #15) > I'm a bit worried this could be costly with the recursive call happening in > l.191 > > Do you think this cleanup can be done in the message add switch block ? > > We wouldh ave to collect all the removed message and then have a cleanup > function call for all of them, which would to the work in a withMutations > callback. Not sure if I understand this correctly but it all happens in MESSAGE_ADD block and within `withMutations` callback. Btw. the new version has `adjustTopLevelMessageCount` method that is called from within the callback and does all the changes to the current `record` object. Honza
Flags: needinfo?(nchevobbe)
(In reply to Alexandre Poirot [:ochameau] from comment #14) > Note that you would have to introduce destroy method on actors, calling > their existing release method. > I don't know why, but today we only do that for LonStringActor... > http://searchfox.org/mozilla-central/source/devtools/server/actors/object. > js#2091 > (These are very old actors that pre-dates protocol.js and many other > evolutions on the server side) > > In your case you should track all the actors created by the webconsole. > To do that, follow the usages of this ActorPool: > http://searchfox.org/mozilla-central/source/devtools/server/actors/ > webconsole.js#58 > - createEnvironmentActor should be your interest as it sounds related to > jsterm > - getNetworkEventActor should be watched if you display network events > - createValueGrip is your main scenario, where all the grip actors are > created. > You should ensure that for any actor added to the pool, you end up > destroying them once they go out of your view limits. Thanks Alex for detailed info! This sounds to me like a follow up (in case we actually want to keep the limit in place). Also, the old console front-end didn't support that. Honza
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #15) > > + record.set("groupsById", record.groupsById.delete(firstMessage.id)); > I'm a bit worried this could be costly with the recursive call happening in > l.191 Fixed in the latest revision Honza
Comment on attachment 8865992 [details] Bug 1307884 - Clean up actors; https://reviewboard.mozilla.org/r/137590/#review141084 ::: devtools/client/webconsole/new-console-output/reducers/messages.js:190 (Diff revision 3) > + }, []); > + > + // Release backend actors. > + actors.forEach(actor => { > + window.proxy.releaseActor(actor); > + }); What is the typical number of actors being released here? If that's a significant number, a "releaseMany" method would be more or less important here.
One thing I'd like to fix in the patch yet is related to how the 'proxy' object is accessed when releasing backend actors. The current solution introduces a global. `NewWebConsoleFrame` object is setting `window.proxy` in `_initConnection` when `WebConsoleConnectionProxy` object is instantiated. Consequently the `messages` reducer is using `window.proxy` when releasing actors in `adjustTopLevelMessageCount` . Introducing globals is obviously not nice and I am wondering what's the better way. We could perhaps move the actors-removal logic into `messageAdd` action creator where it would be easier to access the proxy object. But it would be harder to get the array of removed messages, which is calculated in the reducer. It might be also interesting to introduce a new middle-ware that would be responsible for releasing actors (handling also e.g. the `MESSAGES_CLEAR` action). It would be nicely independent piece of code and we would pass reference to the proxy into it when it's created. The problem how to get the array of removed messages (still calculated in the reducer) is unfortunately still there. Any other ideas? @bgrins: this is what I was talking about at the meeting today. Honza
Flags: needinfo?(nchevobbe)
Here is a summary of current state: 1) 'Limit for message in the store' patch There is a new limit for messages in the store. Only top level messages counts. The oldest message exceeding the limit is removed. The logic might seem complex, but it's responsible for removing all necessary references and it's also nicely wrapped within the `messages` reducer. 2) 'Clean up actors' patch When a message is removed we also need to clean up the backend - remove actors associated with optional message arguments (parameters). This patch introduces a new redux enhancer 'releaseActorsEnhancer' (store.js) that has access to Webconsole connection proxy object. This enhancer gets `removedMessages` array from the store and releases all actors. This array is calculated when removing all messages within the reducers. Removal might happen when MESSAGE_ADD or MESSAGES_CLEAR actionis fired. The enhancer also fires `REMOVED_MESSAGES_CLEAR` action that clears the array. 3) 'Remove prune function' patch The UI doesn't have a limit any more. The only limit is in the store. 4) 'Implement tests' patch Update of existing tests plus new tests introduced. --- Possible follow ups - Release actors using one `releaseMany` packet It believe it's ready for review now. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=73477be70faf85268b962de6838b228d6e2427d2 Honza
Comment on attachment 8866801 [details] Bug 1307884 - Implement tests; https://reviewboard.mozilla.org/r/138408/#review142158 Is it possible here to stub out proxy.releaseActor and ensure it gets called when doing a log with an object that gets removed?
Comment on attachment 8865992 [details] Bug 1307884 - Clean up actors; https://reviewboard.mozilla.org/r/137590/#review142314 Some comments on small things, but overall, I like how this is working. Could we have some store tests to make sure that both the reducer and the "release enhancer" work as expected ? ::: devtools/client/webconsole/new-console-output/new-console-output-wrapper.js:34 (Diff revision 7) > this.owner = owner; > this.document = document; > > this.init = this.init.bind(this); > + > + store = configureStore(this); So i went to the places where we use this and it seems that this is only passed to get the proxy object. I think we could avoid to pass the whole outputWrapper, and instead only pass a config object, which would contain the things we need (for now it would be only the proxy object) ``` configureStore({ proxy: outputWrapper.jsterm.hud.proxy }); ``` This way will allow us to be able to mock a proxy more easily to create the store in the tests (e.g. in http://searchfox.org/mozilla-central/source/devtools/client/webconsole/new-console-output/test/helpers.js#34 ) ::: devtools/client/webconsole/new-console-output/reducers/messages.js:171 (Diff revision 7) > + > + // Remove top level messages logged over the limit. > + let topLevelCount = getToplevelMessageCount(tempRecord); > + while (topLevelCount > logLimit) { > + let removed = removeFirstMessage(tempRecord); > + removedMessages = removedMessages.concat(removed); What do you think of : ``` removeMessages.push(...removeFirstMessage(tempRecord)); ``` so we don't have to do the instantiations ? ::: devtools/client/webconsole/new-console-output/reducers/messages.js:175 (Diff revision 7) > + let removed = removeFirstMessage(tempRecord); > + removedMessages = removedMessages.concat(removed); > + topLevelCount--; > + } > + > + // Filter out messages with no arguments. explain a bit more why we don't need the messages without arguments. (I guess this is because those don't need to release actors, but it'd be better to say it explicitely) ::: devtools/client/webconsole/new-console-output/reducers/messages.js:227 (Diff revision 7) > - removeFirstMessage(record); > + let removed = removeFirstMessage(record); > + removedMessages = removedMessages.concat(removed); Here we could maybe do : ``` removedMessages.push(...removeFirstMessage(record)); ``` ::: devtools/client/webconsole/new-console-output/store.js:85 (Diff revision 7) > + let actors = msg.parameters > + .map(param => param.actor) > + .filter(actor => actor); can we also use reduce to get the actor so we don't have to loop the array twice ? ::: devtools/client/webconsole/new-console-output/store.js:98 (Diff revision 7) > + function releaseActorsEnhancer(state, action) { > + state = reducer(state, action); > + > + let type = action.type; > + let proxy = outputWrapper.jsterm.hud.proxy; > + if (proxy && (type == MESSAGE_ADD || type == MESSAGES_CLEAR)) { > + releaseActors(state.messages.removedMessages, proxy); > + > + // Reset `removedMessages` in message reducer. > + state = reducer(state, { > + type: REMOVED_MESSAGES_CLEAR > + }); > + } > + > + return state; > + } do we need to create this function inside of the one we return ? I guess this could be moved out of it, so we don't re-create it on each state change, but I might be missing something
Comment on attachment 8866750 [details] Bug 1307884 - Remove prune function; https://reviewboard.mozilla.org/r/138358/#review142662
Attachment #8866750 - Flags: review?(bgrinstead) → review+
Comment on attachment 8866801 [details] Bug 1307884 - Implement tests; https://reviewboard.mozilla.org/r/138408/#review142664 Clearing review from requests in Comment 40 and the first point in Comment 41
Attachment #8866801 - Flags: review?(bgrinstead)
Comment on attachment 8864869 [details] Bug 1307884 - Limit for messages in the store; https://reviewboard.mozilla.org/r/136554/#review142668
Attachment #8864869 - Flags: review?(bgrinstead) → review+
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #41) > Comment on attachment 8865992 [details] > Bug 1307884 - Clean up actors; > > https://reviewboard.mozilla.org/r/137590/#review142314 > > Some comments on small things, but overall, I like how this is working. > Could we have some store tests to make sure that both the reducer and the > "release enhancer" work as expected ? Done (new test introduced in "Bug 1307884 - Implement tests;" patch) > > ::: > devtools/client/webconsole/new-console-output/new-console-output-wrapper.js: > 34 > (Diff revision 7) > > this.owner = owner; > > this.document = document; > > > > this.init = this.init.bind(this); > > + > > + store = configureStore(this); > > So i went to the places where we use this and it seems that this is only > passed to get the proxy object. > I think we could avoid to pass the whole outputWrapper, and instead only > pass a config object, which would contain the things we need (for now it > would be only the proxy object) The proxy can't be directly passed in since it doesn't exist yet at the time when the store is created. But, I am passing the `hud` into the store, so it's easier to mock it. > > ``` > configureStore({ > proxy: outputWrapper.jsterm.hud.proxy > }); > ``` > > This way will allow us to be able to mock a proxy more easily to create the > store in the tests (e.g. in > http://searchfox.org/mozilla-central/source/devtools/client/webconsole/new- > console-output/test/helpers.js#34 ) > > ::: devtools/client/webconsole/new-console-output/reducers/messages.js:171 > (Diff revision 7) > > + > > + // Remove top level messages logged over the limit. > > + let topLevelCount = getToplevelMessageCount(tempRecord); > > + while (topLevelCount > logLimit) { > > + let removed = removeFirstMessage(tempRecord); > > + removedMessages = removedMessages.concat(removed); > > What do you think of : > > ``` > removeMessages.push(...removeFirstMessage(tempRecord)); Done > ``` > > so we don't have to do the instantiations ? > > ::: devtools/client/webconsole/new-console-output/reducers/messages.js:175 > (Diff revision 7) > > + let removed = removeFirstMessage(tempRecord); > > + removedMessages = removedMessages.concat(removed); > > + topLevelCount--; > > + } > > + > > + // Filter out messages with no arguments. > > explain a bit more why we don't need the messages without arguments. (I > guess this is because those don't need to release actors, but it'd be better > to say it explicitely) Done > > ::: devtools/client/webconsole/new-console-output/reducers/messages.js:227 > (Diff revision 7) > > - removeFirstMessage(record); > > + let removed = removeFirstMessage(record); > > + removedMessages = removedMessages.concat(removed); > > Here we could maybe do : > > ``` > removedMessages.push(...removeFirstMessage(record)); > ``` Done > > ::: devtools/client/webconsole/new-console-output/store.js:85 > (Diff revision 7) > > + let actors = msg.parameters > > + .map(param => param.actor) > > + .filter(actor => actor); > > can we also use reduce to get the actor so we don't have to loop the array > twice ? I used forEach and removed the second loop. > > ::: devtools/client/webconsole/new-console-output/store.js:98 > (Diff revision 7) > > + function releaseActorsEnhancer(state, action) { > > + state = reducer(state, action); > > + > > + let type = action.type; > > + let proxy = outputWrapper.jsterm.hud.proxy; > > + if (proxy && (type == MESSAGE_ADD || type == MESSAGES_CLEAR)) { > > + releaseActors(state.messages.removedMessages, proxy); > > + > > + // Reset `removedMessages` in message reducer. > > + state = reducer(state, { > > + type: REMOVED_MESSAGES_CLEAR > > + }); > > + } > > + > > + return state; > > + } > > do we need to create this function inside of the one we return ? I guess > this could be moved out of it, so we don't re-create it on each state > change, but I might be missing something Done Thanks for the review! Honza
Iteration: 55.5 - May 15 → 55.6 - May 29
Comment on attachment 8866801 [details] Bug 1307884 - Implement tests; https://reviewboard.mozilla.org/r/138408/#review143208 messages.test.js looks good to me. Nicolas, can you review release-actors.test.js?
Attachment #8866801 - Flags: review?(bgrinstead) → review+
Comment on attachment 8866801 [details] Bug 1307884 - Implement tests; See Comment 50
Attachment #8866801 - Flags: review?(nchevobbe)
Comment on attachment 8866801 [details] Bug 1307884 - Implement tests; https://reviewboard.mozilla.org/r/138408/#review143328 Loks good, thanks ! Could we add a test for messagesClear action too ?
Attachment #8866801 - Flags: review?(nchevobbe) → review+
Comment on attachment 8865992 [details] Bug 1307884 - Clean up actors; https://reviewboard.mozilla.org/r/137590/#review143156 ::: devtools/client/webconsole/new-console-output/reducers/messages.js:97 (Diff revision 8) > - } > - } > }); > case constants.MESSAGES_CLEAR: > return state.withMutations(function (record) { > + record.set("removedMessages", record.messagesById.filter(msg => msg.parameters)); nit: add a comment to explain why we filter things here. Also, is this givin us an array or a new Immutable List ?
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #52) > Comment on attachment 8866801 [details] > Bug 1307884 - Implement tests; > > https://reviewboard.mozilla.org/r/138408/#review143328 > > Loks good, thanks ! > Could we add a test for messagesClear action too ? Done > > - } > > - } > > }); > > case constants.MESSAGES_CLEAR: > > return state.withMutations(function (record) { > > + record.set("removedMessages", record.messagesById.filter(msg => msg.parameters)); > > nit: add a comment to explain why we filter things here. Done > Also, is this givin us an array or a new Immutable List ? Good point, returns array now. Thanks! Honza
Comment on attachment 8865992 [details] Bug 1307884 - Clean up actors; https://reviewboard.mozilla.org/r/137590/#review143782
Attachment #8865992 - Flags: review?(nchevobbe) → review+
I am seeing memory leaks on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5d5c815584eabaefd191a8e390e62b9a486cd13&selectedJob=100017752 For example, this test is failing: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_string.js I can see that the memory leaks are fixed if I don't pass `this.jsterm.hud` into `configureStore` in `NewConsoleOutputWrapper` ctor. It looks like the reference causes the leak holding the parent window. I tried various things, but I can't figure out how to pass hud into the store/middleware correctly. Any tips? Honza
Flags: needinfo?(nchevobbe)
Flags: needinfo?(bgrinstead)
Two ideas: 1) Does it help to pass in proxy directly instead of hud? 2) Maybe we could add a destroy function on NewConsoleOutputWrapper and call it from webconsole.js (and new-webconsole.js which is used in launchpad for now but eventually for the HTML page). Then in there we could tear down the store.
Flags: needinfo?(bgrinstead)
> 1) Does it help to pass in proxy directly instead of hud? I think we can't because the proxy doesn't exist when we create the NewConsoleOutputWrapper. Maybe we can pass an object with a getProxy function so we'll have access to it whenever we want. What do you think of this Honza ?
Flags: needinfo?(nchevobbe)
(In reply to Brian Grinstead [:bgrins] from comment #61) > Two ideas: > > 1) Does it help to pass in proxy directly instead of hud? > 2) Maybe we could add a destroy function on NewConsoleOutputWrapper and call > it from webconsole.js (and new-webconsole.js which is used in launchpad for > now but eventually for the HTML page). Then in there we could tear down the > store. (In reply to Nicolas Chevobbe [:nchevobbe] from comment #62) > > 1) Does it help to pass in proxy directly instead of hud? > I think we can't because the proxy doesn't exist when we create the > NewConsoleOutputWrapper. > Maybe we can pass an object with a getProxy function so we'll have access to > it whenever we want. > What do you think of this Honza ? Thanks for the tips! I have been actually trying all these things (including additional destroy method), but none helped for me. In the end I had to do this: https://reviewboard.mozilla.org/r/137590/diff/9-10/ I don't have good explanation why. Honza
Comment on attachment 8865992 [details] Bug 1307884 - Clean up actors; https://reviewboard.mozilla.org/r/137590/#review145136 ::: devtools/client/webconsole/new-console-output/store.js:88 (Diff revisions 9 - 10) > function releaseActorsEnhancer(state, action) { > state = reducer(state, action); > > let type = action.type; > let proxy = hud ? hud.proxy : null; > - if (proxy && (type == MESSAGE_ADD || type == MESSAGES_CLEAR)) { > + if (type == MESSAGE_ADD || type == MESSAGES_CLEAR) { I guess this will make us ait until the proxy is set before releasing any actors. I hae the feeling that we can afford to do that since 1) it probably doesn't take much time, 2) in the end we will remove the actors of the previous state changes when we didn't have the proxy. ::: devtools/client/webconsole/new-console-output/store.js:113 (Diff revisions 9 - 10) > - msg.parameters.forEach(param => { > - if (param.actor) { > + for (let i = 0; i < msg.parameters.length; i++) { > + let param = msg.parameters[i]; > + if (param && param.actor) { > proxy.releaseActor(param.actor); > } > + } why do we need to switch to a for loop ?
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #68) > Comment on attachment 8865992 [details] > Bug 1307884 - Clean up actors; > > https://reviewboard.mozilla.org/r/137590/#review145136 > > ::: devtools/client/webconsole/new-console-output/store.js:88 > (Diff revisions 9 - 10) > > function releaseActorsEnhancer(state, action) { > > state = reducer(state, action); > > > > let type = action.type; > > let proxy = hud ? hud.proxy : null; > > - if (proxy && (type == MESSAGE_ADD || type == MESSAGES_CLEAR)) { > > + if (type == MESSAGE_ADD || type == MESSAGES_CLEAR) { > > I guess this will make us ait until the proxy is set before releasing any > actors. > I hae the feeling that we can afford to do that since > 1) it probably doesn't take much time, 2) in the end we will remove the > actors of the previous state changes when we didn't have the proxy. You, are right. I reverted the condition back to what it was. > ::: devtools/client/webconsole/new-console-output/store.js:113 > (Diff revisions 9 - 10) > > - msg.parameters.forEach(param => { > > - if (param.actor) { > > + for (let i = 0; i < msg.parameters.length; i++) { > > + let param = msg.parameters[i]; > > + if (param && param.actor) { > > proxy.releaseActor(param.actor); > > } > > + } > > why do we need to switch to a for loop ? Memory leaks. I am not sure, but forEach creates a closure...? Honza
Depends on: 1367266
Depends on: 1389803
No longer depends on: 1389803
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: