Bug 69146 - Web Inspector: debuggerPresentatioModel.linkifyLocation leaks updateAnchor closure instances.
Summary: Web Inspector: debuggerPresentatioModel.linkifyLocation leaks updateAnchor cl...
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: 69268
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-30 06:55 PDT by Ilya Tikhonovsky
Modified: 2011-10-05 04:40 PDT (History)
12 users (show)

See Also:


Attachments
Patch (12.57 KB, patch)
2011-09-30 22:30 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Patch (15.51 KB, patch)
2011-10-03 02:50 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Patch (19.59 KB, patch)
2011-10-03 05:58 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Patch (31.09 KB, patch)
2011-10-03 07:46 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Patch (44.80 KB, patch)
2011-10-04 09:10 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Patch (37.74 KB, patch)
2011-10-04 10:03 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Patch (37.82 KB, patch)
2011-10-04 10:23 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Patch (38.46 KB, patch)
2011-10-05 01:45 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Patch (38.77 KB, patch)
2011-10-05 02:35 PDT, Ilya Tikhonovsky
pfeldman: review+
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-09-30 06:55:27 PDT
In many places we use linkifyLocation function to produce a link node which will updates automatically when the source file was changed on the fly.
Such changes happen when we use pretty print or another operation that changes the source code somehow.

linkifyLocation associates a new instance of updateAnchor closure with the each link node and add the closure to the SourceMappingUpdated event's list.

As result the node<->closure pairs wouldn't be collected until reset the UI and DebuggerPresentationModel
Comment 1 Ilya Tikhonovsky 2011-09-30 22:30:01 PDT
Created attachment 109396 [details]
Patch
Comment 2 Pavel Feldman 2011-09-30 22:35:43 PDT
Comment on attachment 109396 [details]
Patch

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

> Source/WebCore/inspector/front-end/TimelinePanel.js:1181
> +        return WebInspector.debuggerPresentationModel.linkifyLocation(WebInspector.TimelinePanel.linkedNodesOwnerName, url, lineNumber, columnNumber, "timeline-details");

I would form it a bit differently to improve readability.

I would introduce DebuggerPresentationModel.Linkifier and do this:

this._linkifier = DebuggerPresentationModel.createLinkifier();
this._linkifier.linkifyLocation(url, lineNumber, ...)
this._linkifier.reset() (or delete this._linkifier);
Comment 3 Ilya Tikhonovsky 2011-09-30 22:46:56 PDT
(In reply to comment #2)
> (From update of attachment 109396 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109396&action=review
> 
> > Source/WebCore/inspector/front-end/TimelinePanel.js:1181
> > +        return WebInspector.debuggerPresentationModel.linkifyLocation(WebInspector.TimelinePanel.linkedNodesOwnerName, url, lineNumber, columnNumber, "timeline-details");
> 
> I would form it a bit differently to improve readability.
> 
> I would introduce DebuggerPresentationModel.Linkifier and do this:
> 
> this._linkifier = DebuggerPresentationModel.createLinkifier();
> this._linkifier.linkifyLocation(url, lineNumber, ...)
> this._linkifier.reset() (or delete this._linkifier);

Panel:
WebInspector.TimelinePanel.linkifier = DebuggerPresentationModel.createLinkifier();

lalalalNode:
WebInspector.TimelinePanel.linkifier.linkifyLocation(url, lineNumber, ...)

panel.reset:
WebInspector.TimelinePanel.linkifier.reset() (or delete this._linkifier);
Comment 4 Pavel Feldman 2011-09-30 22:48:39 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 109396 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=109396&action=review
> > 
> > > Source/WebCore/inspector/front-end/TimelinePanel.js:1181
> > > +        return WebInspector.debuggerPresentationModel.linkifyLocation(WebInspector.TimelinePanel.linkedNodesOwnerName, url, lineNumber, columnNumber, "timeline-details");
> > 
> > I would form it a bit differently to improve readability.
> > 
> > I would introduce DebuggerPresentationModel.Linkifier and do this:
> > 
> > this._linkifier = DebuggerPresentationModel.createLinkifier();
> > this._linkifier.linkifyLocation(url, lineNumber, ...)
> > this._linkifier.reset() (or delete this._linkifier);
> 
> Panel:
> WebInspector.TimelinePanel.linkifier = DebuggerPresentationModel.createLinkifier();

you'll need to create this on instance of dpm. I would do WebInspector.debuggerPresentationModel.createLinkifier from within TimelinePanel constructor.


> 


> lalalalNode:
> WebInspector.TimelinePanel.linkifier.linkifyLocation(url, lineNumber, ...)

should be defined on instance (this._linkifier ...)
> 
> panel.reset:
> WebInspector.TimelinePanel.linkifier.reset() (or delete this._linkifier);
Comment 5 Pavel Feldman 2011-10-02 10:21:26 PDT
Comment on attachment 109396 [details]
Patch

r- as per comments in the bug
Comment 6 Ilya Tikhonovsky 2011-10-03 02:50:28 PDT
Created attachment 109457 [details]
Patch
Comment 7 WebKit Review Bot 2011-10-03 03:11:19 PDT
Comment on attachment 109457 [details]
Patch

Attachment 109457 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9937004

New failing tests:
http/tests/inspector/network/network-disabling-check-no-memory-leak.html
http/tests/inspector/console-xhr-logging.html
http/tests/inspector/network/network-cachedresources-with-same-urls.html
http/tests/inspector/console-cd.html
http/tests/inspector/network/network-disable-cache-memory.html
http/tests/inspector/resource-har-headers.html
http/tests/inspector/network/network-disable-cache-xhrs.html
http/tests/inspector/network/download.html
http/tests/inspector-enabled/console-clear-arguments-on-frame-remove.html
http/tests/inspector/inspect-iframe-from-different-domain.html
http/tests/inspector-enabled/console-log-before-frame-navigation.html
http/tests/inspector/network-preflight-options.html
http/tests/inspector/network/network-content-replacement-xhr.html
http/tests/inspector/console-cd-completions.html
http/tests/inspector-enabled/dom-storage-open.html
http/tests/inspector/resource-har-conversion.html
http/tests/inspector/console-resource-errors.html
http/tests/inspector/resource-parameters.html
http/tests/inspector-enabled/database-open.html
http/tests/inspector/change-iframe-src.html
Comment 8 Ilya Tikhonovsky 2011-10-03 05:58:06 PDT
Created attachment 109477 [details]
Patch
Comment 9 Pavel Feldman 2011-10-03 06:03:51 PDT
Comment on attachment 109477 [details]
Patch

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

> Source/WebCore/WebCore.gypi:6237
> +            'inspector/front-end/Linkifier.js',

Do not forget about .vcproj!

> Source/WebCore/inspector/front-end/EventListenersSidebarPane.js:59
> +    WebInspector.EventListenersSidebarPane._linkifier = new WebInspector.Linkifier();

you should install these on instance. Installing on class in constructor does not make sense - it leaks linkifier anyways.

> Source/WebCore/inspector/front-end/Linkifier.js:46
> +        var rawSourceCode = WebInspector.debuggerPresentationModel._rawSourceCodeForScript(sourceURL);

Extracting linkifier in its own class does not remote dependency from the DPM. Not sure what you were trying to achieve here.

> Source/WebCore/inspector/front-end/ProfileDataGridTree.js:101
> +            var urlElement = WebInspector.ProfilesPanel.linkifier.linkifyLocation(this.profileNode.url, lineNumber, 0, "profile-node-file");

Why not to create linkifier on the tree instead?

> Source/WebCore/inspector/front-end/TimelinePanel.js:112
> +    WebInspector.TimelinePanel._linkifier = new WebInspector.Linkifier();

ditto
Comment 10 Ilya Tikhonovsky 2011-10-03 07:46:18 PDT
Created attachment 109483 [details]
Patch
Comment 11 Yury Semikhatsky 2011-10-03 07:52:22 PDT
Comment on attachment 109483 [details]
Patch

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

> Source/WebCore/inspector/front-end/Linkifier.js:36
> +    this._model = model;

_model -> _debuggerPresentationModel, alternatively you may add corresponding type annotation.
Comment 12 Ilya Tikhonovsky 2011-10-03 08:06:09 PDT
Committed r96500: <http://trac.webkit.org/changeset/96500>
Comment 13 Ilya Tikhonovsky 2011-10-04 09:10:34 PDT
Created attachment 109628 [details]
Patch
Comment 14 Pavel Feldman 2011-10-04 09:27:37 PDT
Comment on attachment 109628 [details]
Patch

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

> LayoutTests/inspector/console/console-preserve-log.html:8
> +    WebInspector.console.addMessage(WebInspector.consoleView.createTextMessage("PASS"));

You should always use factory method when creating messages.

> LayoutTests/inspector/debugger/linkifier.html:37
> +        DummyPresentationModel.prototype = {

Something is wrong with indentation.

> Source/WebCore/inspector/front-end/ConsoleMessage.js:37
> + * @param {WebInspector.DebuggerPresentationModel.Linkifier=} linkifier

move this parameter to after "message", make it non-optional

> Source/WebCore/inspector/front-end/ConsoleModel.js:126
> +            var msgCopy = WebInspector.consoleView.createMessage(msg.source, msg.type, msg.level, msg.line, msg.url, count - prevRepeatCount, msg._messageText, msg._parameters, msg._stackTrace, msg._request);

ditto

> Source/WebCore/inspector/front-end/ConsoleModel.js:177
> +WebInspector.ConsoleMessage.create = function(source, type, level, line, url, repeatCount, message, parameters, stackTrace, request, linkifier)

These should not have linkifier parameter.

> Source/WebCore/inspector/front-end/ConsoleModel.js:187
> +WebInspector.ConsoleMessage.createTextMessage = function(text, level, linkifier)

ditto

> Source/WebCore/inspector/front-end/ConsoleView.js:123
> +    createMessage: function(source, type, level, line, url, repeatCount, message, parameters, stackTrace, request)

WebInspector.ConsoleMessage.create should be defined in the bottom of this class. You will add WebInspector.consoleView.linkifier() into each of the created ConsoleMessageImpls.

> Source/WebCore/inspector/front-end/ConsoleView.js:129
> +    {

createTextMessage

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:404
> +    rawSourceCodeForScriptWithURL: function(sourceURL)

This can't be exposed.

> Source/WebCore/inspector/front-end/ExtensionServer.js:333
> +        var consoleMessage = WebInspector.consoleView.createMessage(

ditto

> Source/WebCore/inspector/front-end/NetworkManager.js:-130
> -            WebInspector.console.addMessage(WebInspector.ConsoleMessage.create(WebInspector.ConsoleMessage.MessageSource.Other,

ditt

> Source/WebCore/inspector/front-end/ProfileView.js:490
> +        this._linkiier.reset();

This should not work!

> Source/WebCore/inspector/front-end/inspector.js:1012
> +        var msg = WebInspector.consoleView.createMessage(

ditto
Comment 15 Ilya Tikhonovsky 2011-10-04 10:03:33 PDT
Created attachment 109634 [details]
Patch
Comment 16 Pavel Feldman 2011-10-04 10:15:21 PDT
Comment on attachment 109634 [details]
Patch

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

> LayoutTests/inspector/debugger/linkifier.html:49
> +        console.log("--- linkify fakeURL-1 first time ---");

console.log -> InspectorTest.addResult

> Source/WebCore/inspector/front-end/ConsoleView.js:701
> +    WebInspector.ConsoleCommandResult = function(result, wasThrown, originatingCommand, linkifier)

Weird indent
Comment 17 Ilya Tikhonovsky 2011-10-04 10:21:47 PDT
Comment on attachment 109634 [details]
Patch

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

>> Source/WebCore/inspector/front-end/ConsoleView.js:701
>> +    WebInspector.ConsoleCommandResult = function(result, wasThrown, originatingCommand, linkifier)
> 
> Weird indent

it was tab
Comment 18 Ilya Tikhonovsky 2011-10-04 10:23:41 PDT
Created attachment 109639 [details]
Patch
Comment 19 Pavel Podivilov 2011-10-04 10:37:11 PDT
Comment on attachment 109634 [details]
Patch

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

Looks good.

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:565
> +    this._anchorsForURL = {};

We should be able to linkify console messages for anonymous scripts as well. Could you store anchors by rawSourceCode.id instead so we can easily support anonymous scripts?

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:609
> +        if (!anchors)

How could this happen?
Comment 20 Pavel Feldman 2011-10-04 23:51:58 PDT
Comment on attachment 109639 [details]
Patch

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

Overall looks good. Could you update it and let Pavel P. take a look?

> LayoutTests/inspector/debugger/linkifier.html:21
> +        DummyPresentationModel = function()

You should define classes according the the style guidelines.

> LayoutTests/inspector/debugger/linkifier.html:25
> +                this.addEventListener = function(eventName, callback, object)

ditto

> LayoutTests/inspector/debugger/linkifier.html:37
> +        DummyPresentationModel.prototype = {

ditto

> Source/WebCore/inspector/front-end/ConsoleView.js:701
> +    WebInspector.ConsoleCommandResult = function(result, wasThrown, originatingCommand, linkifier)

Weird space added. It is clear that it has been added, not removed.
Comment 21 Ilya Tikhonovsky 2011-10-05 01:45:48 PDT
Created attachment 109763 [details]
Patch
Comment 22 Ilya Tikhonovsky 2011-10-05 01:59:06 PDT
the landed patch was rolled out.
reopen the bug.
Comment 23 Pavel Podivilov 2011-10-05 02:09:33 PDT
Comment on attachment 109763 [details]
Patch

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

Looks good.

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:565
> +    this._anchorsForScriptId = {};

Please rename to anchorsForRawSourceCode or anchorsForRawSourceCodeId, because rawSourceCode.id is not a scriptId.

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:599
> +            if (rawSourceCode)

This check is not needed. You add entries in this._anchorsForScriptId only when rawSourceCode exists, and model._rawSourceCode is cleared only on navigation.
Comment 24 Ilya Tikhonovsky 2011-10-05 02:35:16 PDT
Created attachment 109765 [details]
Patch
Comment 25 Pavel Feldman 2011-10-05 03:10:10 PDT
Comment on attachment 109765 [details]
Patch

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

> LayoutTests/inspector/debugger/linkifier.html:45
> +            _rawSourceCodeForScript: function(id)

nit: it should be "script".
Comment 26 Ilya Tikhonovsky 2011-10-05 04:40:17 PDT
Committed r96694: <http://trac.webkit.org/changeset/96694>