Bug 69744 – Web Inspector: small memory leak in scripts panel

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Ilya Tikhonovsky]

Scripts panel doesn't clean the list of child views.

[Ilya Tikhonovsky]

Created attachment 110334 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 110334 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110334&action=review

> Source/WebCore/inspector/front-end/Panel.js:100
> +        WebInspector.View.prototype.reset.call(this);

This may well break some panels. We shouldn't remove all child views on reset I think. Can you do this clean-up in the ScriptsPanel for now?

> Source/WebCore/inspector/front-end/View.js:158
> +    reset: function()

I'd rather renamed this to something like removeAllChildView

[Andrey Kosyakov]

Comment on attachment 110334 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110334&action=review

> Source/WebCore/inspector/front-end/View.js:165
> +        for (var i = 0; i < this._children.length; ++i) {
> +            var view = this._children[i];
> +            view._parentView = null;
> +            view._detach();
> +        }
> +        this._children = [];

We shouldn't destroy view hierarchy upon reset, reset is a model event that typically causes model objects be removed, but does not necessarily affect UI objects (consider NetworkLogView, which is not removed upon reset(), although may be cleaned). There does not seem to be a valid reason for View.reset() to exist at all. I suggest we rather handler view removal in the higher-level code, symmetrical to how the particular view is created.

[Ilya Tikhonovsky]

Created attachment 110337 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 110337 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110337&action=review

> Source/WebCore/inspector/front-end/ScriptsPanel.js:569
> +        this._detachSourceFramesViews();

Why don't we get an event upon script removal from the model instead? We already have UISourceCodeAdded and UISourceCodeReplaced, it would seem natural to have UISourceCodeRemoved. Besides, DebuggerPresentationModel already handles reset event and cleans up the source code accordingly.

[Ilya Tikhonovsky]

Created attachment 110342 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 110342 [details]
Patch

LGTM

[Yury Semikhatsky]

Comment on attachment 110342 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110342&action=review

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:435
> +            this.dispatchEventToListeners(WebInspector.DebuggerPresentationModel.Events.UISourceCodeRemoved, this._rawSourceCode[id].sourceMapping.uiSourceCode);

Pavel Podivilov was going to allow for multiple UISourceCode's per single RawSourceCode, but I assume it is not landed yet. Also we may want to introduce something like UISourceCodesCleared event for performance reasons., but for now this approach is fine.

[Ilya Tikhonovsky]

Committed r97050: <http://trac.webkit.org/changeset/97050>

[Csaba Osztrogonác]

It broke inspector/debugger/script-formatter.html on the SL and on the Qt bot:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20%28Tests%29/r97050%20%2833774%29/inspector/debugger/script-formatter-pretty-diff.html

Reopen to fix it.

[Csaba Osztrogonác]

Comment on attachment 110342 [details]
Patch

remove r+ from landed patch

[Csaba Osztrogonác]

Fix landed in http://trac.webkit.org/changeset/97055