Bug 69744 - Web Inspector: small memory leak in scripts panel
Summary: Web Inspector: small memory leak in scripts panel
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Ilya Tikhonovsky
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-10 00:16 PDT by Ilya Tikhonovsky
Modified: 2011-10-10 05:49 PDT (History)
12 users (show)

See Also:


Attachments
Patch (2.96 KB, patch)
2011-10-10 00:19 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Patch (2.03 KB, patch)
2011-10-10 01:39 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Patch (4.00 KB, patch)
2011-10-10 02:33 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ilya Tikhonovsky 2011-10-10 00:16:29 PDT
Scripts panel doesn't clean the list of child views.
Comment 1 Ilya Tikhonovsky 2011-10-10 00:19:39 PDT
Created attachment 110334 [details]
Patch
Comment 2 Yury Semikhatsky 2011-10-10 00:49:58 PDT
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
Comment 3 Andrey Kosyakov 2011-10-10 01:06:02 PDT
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.
Comment 4 Ilya Tikhonovsky 2011-10-10 01:39:45 PDT
Created attachment 110337 [details]
Patch
Comment 5 Andrey Kosyakov 2011-10-10 01:45:51 PDT
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.
Comment 6 Ilya Tikhonovsky 2011-10-10 02:33:15 PDT
Created attachment 110342 [details]
Patch
Comment 7 Andrey Kosyakov 2011-10-10 02:34:17 PDT
Comment on attachment 110342 [details]
Patch

LGTM
Comment 8 Yury Semikhatsky 2011-10-10 02:39:57 PDT
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.
Comment 9 Ilya Tikhonovsky 2011-10-10 02:53:12 PDT
Committed r97050: <http://trac.webkit.org/changeset/97050>
Comment 10 Csaba Osztrogonác 2011-10-10 03:55:46 PDT
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.
Comment 11 Csaba Osztrogonác 2011-10-10 03:56:08 PDT
Comment on attachment 110342 [details]
Patch

remove r+ from landed patch
Comment 12 Csaba Osztrogonác 2011-10-10 05:49:31 PDT
Fix landed in http://trac.webkit.org/changeset/97055