Summary: | Web Inspector: debuggerPresentatioModel.linkifyLocation leaks updateAnchor closure instances. | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ilya Tikhonovsky <loislo> | ||||||||||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Ilya Tikhonovsky <loislo> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | apavlov, bweinstein, dglazkov, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||
Bug Depends on: | 69268 | ||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||
Attachments: |
|
Description
Ilya Tikhonovsky
2011-09-30 06:55:27 PDT
Created attachment 109396 [details]
Patch
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); (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); (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 on attachment 109396 [details]
Patch
r- as per comments in the bug
Created attachment 109457 [details]
Patch
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 Created attachment 109477 [details]
Patch
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 Created attachment 109483 [details]
Patch
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. Committed r96500: <http://trac.webkit.org/changeset/96500> Created attachment 109628 [details]
Patch
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 Created attachment 109634 [details]
Patch
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 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 Created attachment 109639 [details]
Patch
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 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. Created attachment 109763 [details]
Patch
the landed patch was rolled out. reopen the bug. 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. Created attachment 109765 [details]
Patch
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". Committed r96694: <http://trac.webkit.org/changeset/96694> |