Bug 69146 – Web Inspector: debuggerPresentatioModel.linkifyLocation leaks updateAnchor closure instances.

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

[Ilya Tikhonovsky]

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

[Ilya Tikhonovsky]

Created attachment 109396 [details]
Patch

[Pavel Feldman]

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);

[Ilya Tikhonovsky]

(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);

[Pavel Feldman]

(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);

[Pavel Feldman]

Comment on attachment 109396 [details]
Patch

r- as per comments in the bug

[Ilya Tikhonovsky]

Created attachment 109457 [details]
Patch

[WebKit Review Bot]

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

[Ilya Tikhonovsky]

Created attachment 109477 [details]
Patch

[Pavel Feldman]

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

[Ilya Tikhonovsky]

Created attachment 109483 [details]
Patch

[Yury Semikhatsky]

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.

[Ilya Tikhonovsky]

Committed r96500: <http://trac.webkit.org/changeset/96500>

[Ilya Tikhonovsky]

Created attachment 109628 [details]
Patch

[Pavel Feldman]

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

[Ilya Tikhonovsky]

Created attachment 109634 [details]
Patch

[Pavel Feldman]

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

[Ilya Tikhonovsky]

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

[Ilya Tikhonovsky]

Created attachment 109639 [details]
Patch

[Pavel Podivilov]

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?

[Pavel Feldman]

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.

[Ilya Tikhonovsky]

Created attachment 109763 [details]
Patch

[Ilya Tikhonovsky]

the landed patch was rolled out.
reopen the bug.

[Pavel Podivilov]

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.

[Ilya Tikhonovsky]

Created attachment 109765 [details]
Patch

[Pavel Feldman]

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".

[Ilya Tikhonovsky]

Committed r96694: <http://trac.webkit.org/changeset/96694>