Bug 20469 – Close leak of PausedTimeouts, and clean up ownership

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

Close leak of PausedTimeouts, and clean up ownership

See attached patch.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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

[Darin Adler]

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!

[Darin Adler]

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!

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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

[Alexey Proskuryakov]

Comment on attachment 23052 [details]
Updated patch with darin's suggested changes

r=me, based on Darin's earlier review.

[Eric Seidel (no email)]

Thanks to both of you!  r35964