Bug 85009 - ScriptStateProtectedPtr should not keep a strong reference to the context
Summary: ScriptStateProtectedPtr should not keep a strong reference to the context
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yury Semikhatsky
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-26 14:47 PDT by jochen
Modified: 2012-04-28 03:30 PDT (History)
6 users (show)

See Also:


Attachments
Patch (17.17 KB, patch)
2012-04-27 11:11 PDT, Yury Semikhatsky
pfeldman: review+
gustavo: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jochen 2012-04-26 14:47:19 PDT
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.
Comment 1 jochen 2012-04-27 01:49:11 PDT
+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?
Comment 2 Pavel Feldman 2012-04-27 03:12:30 PDT
(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.
Comment 3 Yury Semikhatsky 2012-04-27 05:22:35 PDT
(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?
Comment 4 jochen 2012-04-27 05:32:49 PDT
(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?
Comment 5 Yury Semikhatsky 2012-04-27 05:35:58 PDT
Also all messages are cleared on the main frame navigation. Closing the bug.
Comment 6 Yury Semikhatsky 2012-04-27 06:01:12 PDT
(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.
Comment 7 jochen 2012-04-27 06:05:00 PDT
(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.
Comment 8 Yury Semikhatsky 2012-04-27 06:51:15 PDT
(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.
Comment 9 Yury Semikhatsky 2012-04-27 11:11:39 PDT
Created attachment 139235 [details]
Patch
Comment 10 Gustavo Noronha (kov) 2012-04-27 11:27:02 PDT
Comment on attachment 139235 [details]
Patch

Attachment 139235 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12566127
Comment 11 Build Bot 2012-04-27 11:49:54 PDT
Comment on attachment 139235 [details]
Patch

Attachment 139235 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12548281
Comment 12 Build Bot 2012-04-27 12:06:17 PDT
Comment on attachment 139235 [details]
Patch

Attachment 139235 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12558284
Comment 13 Pavel Feldman 2012-04-28 00:35:04 PDT
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 14 Yury Semikhatsky 2012-04-28 01:18:46 PDT
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.
Comment 15 Yury Semikhatsky 2012-04-28 01:19:46 PDT
Committed r115553: <http://trac.webkit.org/changeset/115553>
Comment 16 Philippe Normand 2012-04-28 03:22:42 PDT
(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?
Comment 17 Philippe Normand 2012-04-28 03:30:53 PDT
(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.