Bug 85009 – ScriptStateProtectedPtr should not keep a strong reference to the context

• Component: WebCore Misc.
• Status: RESOLVED FIXED

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?

[Pavel Feldman]

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

[Yury Semikhatsky]

(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?

[Yury Semikhatsky]

Also all messages are cleared on the main frame navigation. Closing the bug.

[Yury Semikhatsky]

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

[Yury Semikhatsky]

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

[Yury Semikhatsky]

Created attachment 139235 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 139235 [details]
Patch

Attachment 139235 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12566127

[Build Bot]

Comment on attachment 139235 [details]
Patch

Attachment 139235 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12548281

[Build Bot]

Comment on attachment 139235 [details]
Patch

Attachment 139235 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12558284

[Pavel Feldman]

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

[Yury Semikhatsky]

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.

[Yury Semikhatsky]

Committed r115553: <http://trac.webkit.org/changeset/115553>

[Philippe Normand]

(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?

[Philippe Normand]

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