ScriptStateProtectedPtr keeps a strong ref to the javascript context. It's only used from the Inspector to store script arguments of console messages. While only a certain amount of console message (1000) are kept, this can keep up to 1000 different contexts alive which might easily exceed the available memory. I think instead, the ScriptStateProtectedPtr should keep a weak reference, and the inspector should cope with the arguments being gone when the context died.
+pavel I see three ways to fix this: 1) discard console messages when the context goes away 2) keep the console messages, but cope with the script arguments maybe not being there 3) instead of retaining the whole context, only retain the individual arguments I think 2) would be best, wdyt?
(In reply to comment #1) > +pavel > > I see three ways to fix this: > > 1) discard console messages when the context goes away > 2) keep the console messages, but cope with the script arguments maybe not being there > 3) instead of retaining the whole context, only retain the individual arguments > > I think 2) would be best, wdyt? I was under the impression that (2) was already taking place. I'll defer to Yury since he was implementing it.
(In reply to comment #2) > I was under the impression that (2) was already taking place. I'll defer to Yury since he was implementing it. The message arguments should be discarded on window navigation. See ConsoleMessage::windowCleared: http://code.google.com/p/chromium/source/search?q=ConsoleMessage%3A%3AwindowCleared Any evidences that it doesn't happen?
(In reply to comment #3) > (In reply to comment #2) > > I was under the impression that (2) was already taking place. I'll defer to Yury since he was implementing it. > > The message arguments should be discarded on window navigation. See ConsoleMessage::windowCleared: http://code.google.com/p/chromium/source/search?q=ConsoleMessage%3A%3AwindowCleared Any evidences that it doesn't happen? If I'm reading this correctly, that code path is only invoked when page is detached, but not when an iframe navigates?
Also all messages are cleared on the main frame navigation. Closing the bug.
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > If I'm reading this correctly, that code path is only invoked when page is detached, but not when an iframe navigates? It should be called on navigation as well but as you pointed out offline there is a scenario when we won't clear the argument and return early from ConsoleMessage::windowCleared because of the following code: if (domWindowFromScriptState(m_arguments->globalState()) != window) return; Reopening the bug.
(In reply to comment #6) > (In reply to comment #4) > > (In reply to comment #3) > > > (In reply to comment #2) > > If I'm reading this correctly, that code path is only invoked when page is detached, but not when an iframe navigates? > > It should be called on navigation as well but as you pointed out offline there is a scenario when we won't clear the argument and return early from ConsoleMessage::windowCleared because of the following code: > > if (domWindowFromScriptState(m_arguments->globalState()) != window) > return; I added fprintf(stderr, "window cleared\n"); to ConsoleMessage::windowCleared and ran LayoutTests/http/tests/inspector-enabled/console-log-before-frame-navigation.html, however, the fprintf wasn't triggered during the test. I think binding the clearing of messages to the page destruction can't work.
(In reply to comment #1) > +pavel > > I see three ways to fix this: > > 1) discard console messages when the context goes away > 2) keep the console messages, but cope with the script arguments maybe not being there > 3) instead of retaining the whole context, only retain the individual arguments > The problem is that we need to get the context while formatting the message. Also an argument may well keep reference to the context and keeping strong reference to the argument would keep the context alive. So it doesn't seem to make sense to have weak ref to the context and strong refs to the arguments. At the same time having weak refs to the arguments we cannot be sure that they are not collected. So the current approach when we discard arguments along with the context on frame navigation and clear all messages on the main frame navigation seems ok. We need to fix the scenario described above though.
Created attachment 139235 [details] Patch
Comment on attachment 139235 [details] Patch Attachment 139235 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12566127
Comment on attachment 139235 [details] Patch Attachment 139235 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12548281
Comment on attachment 139235 [details] Patch Attachment 139235 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12558284
Comment on attachment 139235 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139235&action=review > Source/WebCore/inspector/ConsoleMessage.cpp:166 > + Please revert > Source/WebCore/inspector/ConsoleMessage.cpp:204 > + Please revert
Comment on attachment 139235 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139235&action=review >> Source/WebCore/inspector/ConsoleMessage.cpp:166 >> + > > Please revert Done. >> Source/WebCore/inspector/ConsoleMessage.cpp:204 >> + > > Please revert Done.
Committed r115553: <http://trac.webkit.org/changeset/115553>
(In reply to comment #15) > Committed r115553: <http://trac.webkit.org/changeset/115553> Why was this landed knowing 3 (yes, 3!) EWS bots _failed_ build this patch? And no follow-up build fix either?
(In reply to comment #16) > (In reply to comment #15) > > Committed r115553: <http://trac.webkit.org/changeset/115553> > > Why was this landed knowing 3 (yes, 3!) EWS bots _failed_ build this patch? > And no follow-up build fix either? Ok there are build fixes indeed. But still, landing with red EWS is bad, IMHO.