Bug 20469 - Close leak of PausedTimeouts, and clean up ownership
Summary: Close leak of PausedTimeouts, and clean up ownership
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-08-20 16:54 PDT by Eric Seidel (no email)
Modified: 2008-08-28 04:42 PDT (History)
2 users (show)

See Also:


Attachments
First attempt at PausedTimeouts cleanup (9.99 KB, patch)
2008-08-20 16:58 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
First attempt at PausedTimeouts cleanup (10.14 KB, patch)
2008-08-20 17:01 PDT, Eric Seidel (no email)
darin: review-
Details | Formatted Diff | Diff
Updated patch with darin's suggested changes (10.48 KB, patch)
2008-08-28 04:20 PDT, Eric Seidel (no email)
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2008-08-20 16:54:36 PDT
Close leak of PausedTimeouts, and clean up ownership

See attached patch.
Comment 1 Eric Seidel (no email) 2008-08-20 16:58:53 PDT
Created attachment 22907 [details]
First attempt at PausedTimeouts cleanup

 WebCore/ChangeLog                        |   31 ++++++++++++++++++++++++++++++
 WebCore/bindings/js/JSDOMWindowBase.cpp  |   21 ++++++++++---------
 WebCore/bindings/js/JSDOMWindowBase.h    |    5 ++-
 WebCore/bindings/js/ScriptController.cpp |   11 ++++++++++
 WebCore/bindings/js/ScriptController.h   |    3 ++
 WebCore/history/CachedPage.cpp           |    4 +-
 WebCore/page/Chrome.cpp                  |   24 ++++++++--------------
 WebCore/page/JavaScriptDebugServer.cpp   |   14 +++++++++---
 8 files changed, 80 insertions(+), 33 deletions(-)
Comment 2 Eric Seidel (no email) 2008-08-20 17:01:25 PDT
Created attachment 22908 [details]
First attempt at PausedTimeouts cleanup

 WebCore/ChangeLog                        |   34 ++++++++++++++++++++++++++++++
 WebCore/bindings/js/JSDOMWindowBase.cpp  |   21 +++++++++--------
 WebCore/bindings/js/JSDOMWindowBase.h    |    5 ++-
 WebCore/bindings/js/ScriptController.cpp |   11 +++++++++
 WebCore/bindings/js/ScriptController.h   |    3 ++
 WebCore/history/CachedPage.cpp           |    4 +-
 WebCore/page/Chrome.cpp                  |   24 ++++++++-------------
 WebCore/page/JavaScriptDebugServer.cpp   |   14 ++++++++---
 8 files changed, 83 insertions(+), 33 deletions(-)
Comment 3 Darin Adler 2008-08-22 10:47:16 PDT
Comment on attachment 22908 [details]
First attempt at PausedTimeouts cleanup

Good change!

+    void resumeTimeouts(OwnPtr<PausedTimeouts>&);

You're declaring this in ScriptController.h, but I don't see an implemention in ScriptController.cpp.

-    size_t count = m_deferredFrames.size();
-    for (size_t i = 0; i < count; ++i)
+    for (size_t i = 0; i < m_deferredFrames.size(); ++i)

Why this change? It makes the loop potentially slower by putting the calls to size() inside the loop instead of outside. Is there a benefit to the change?

+        OwnPtr<PausedTimeouts> timeouts = m_pausedTimeouts[i].second;

+            OwnPtr<PausedTimeouts> timeouts(m_pausedTimeouts.take(frame));

Why did you use assignment syntax in the first case, but not the second. Consistent would be better.

I'm going to say review- because of the resumeTimeouts issue. I think the patch won't build!
Comment 4 Darin Adler 2008-08-22 10:47:16 PDT
Comment on attachment 22908 [details]
First attempt at PausedTimeouts cleanup

Good change!

+    void resumeTimeouts(OwnPtr<PausedTimeouts>&);

You're declaring this in ScriptController.h, but I don't see an implemention in ScriptController.cpp.

-    size_t count = m_deferredFrames.size();
-    for (size_t i = 0; i < count; ++i)
+    for (size_t i = 0; i < m_deferredFrames.size(); ++i)

Why this change? It makes the loop potentially slower by putting the calls to size() inside the loop instead of outside. Is there a benefit to the change?

+        OwnPtr<PausedTimeouts> timeouts = m_pausedTimeouts[i].second;

+            OwnPtr<PausedTimeouts> timeouts(m_pausedTimeouts.take(frame));

Why did you use assignment syntax in the first case, but not the second. Consistent would be better.

I'm going to say review- because of the resumeTimeouts issue. I think the patch won't build!
Comment 5 Eric Seidel (no email) 2008-08-28 04:18:14 PDT
(In reply to comment #4)
> (From update of attachment 22908 [details] [edit])
> Good change!
> 
> +    void resumeTimeouts(OwnPtr<PausedTimeouts>&);
> 
> You're declaring this in ScriptController.h, but I don't see an implemention in
> ScriptController.cpp.

Yeah, it was missing.  Oops!

> -    size_t count = m_deferredFrames.size();
> -    for (size_t i = 0; i < count; ++i)
> +    for (size_t i = 0; i < m_deferredFrames.size(); ++i)
> 
> Why this change? It makes the loop potentially slower by putting the calls to
> size() inside the loop instead of outside. Is there a benefit to the change?

It made the code smaller, and I know that Vector is a template, and thus all methods must be inlined, so the count is just a member access thus there is no function call overhead.  Perhaps that's cheating?  I can change it back if you feel strongly.

> +        OwnPtr<PausedTimeouts> timeouts = m_pausedTimeouts[i].second;
> 
> +            OwnPtr<PausedTimeouts> timeouts(m_pausedTimeouts.take(frame));
> 
> Why did you use assignment syntax in the first case, but not the second.
> Consistent would be better.

Fixed.

> I'm going to say review- because of the resumeTimeouts issue. I think the patch
> won't build!

Yup.  Builds and runs fine on Mac, but wouldn't have on Windows!  Fixed.
Comment 6 Eric Seidel (no email) 2008-08-28 04:20:52 PDT
Created attachment 23052 [details]
Updated patch with darin's suggested changes

 WebCore/ChangeLog                        |   35 ++++++++++++++++++++++++++++++
 WebCore/bindings/js/JSDOMWindowBase.cpp  |   21 +++++++++--------
 WebCore/bindings/js/JSDOMWindowBase.h    |    5 ++-
 WebCore/bindings/js/ScriptController.cpp |   22 ++++++++++++++++++
 WebCore/bindings/js/ScriptController.h   |    3 ++
 WebCore/history/CachedPage.cpp           |    4 +-
 WebCore/page/Chrome.cpp                  |   26 ++++++++-------------
 WebCore/page/JavaScriptDebugServer.cpp   |   14 ++++++++---
 8 files changed, 96 insertions(+), 34 deletions(-)
Comment 7 Eric Seidel (no email) 2008-08-28 04:23:19 PDT
(In reply to comment #5)
> (In reply to comment #4)
> ... thus all
> methods must be inlined, so the count is just a member access thus there is no
> function call overhead. 

I meant that all methods must be declared in a header, and thus highly likely to be inlined.  I guess I'm choosing more and more code size and cleanliness over possible speed optimizations these days.
Comment 8 Alexey Proskuryakov 2008-08-28 04:38:23 PDT
Comment on attachment 23052 [details]
Updated patch with darin's suggested changes

r=me, based on Darin's earlier review.
Comment 9 Eric Seidel (no email) 2008-08-28 04:42:55 PDT
Thanks to both of you!  r35964