Bug 38112 - [Qt] QScriptValue::toString has a memory leak.
Summary: [Qt] QScriptValue::toString has a memory leak.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jędrzej Nowacki
URL:
Keywords: Qt
Depends on:
Blocks: 31863
  Show dependency treegraph
 
Reported: 2010-04-26 04:12 PDT by Jędrzej Nowacki
Modified: 2010-05-04 06:54 PDT (History)
5 users (show)

See Also:


Attachments
Fix v1 (1.49 KB, patch)
2010-04-26 04:21 PDT, Jędrzej Nowacki
eric: review-
eric: commit-queue-
Details | Formatted Diff | Diff
Fix v2 (1.69 KB, patch)
2010-05-03 03:45 PDT, Jędrzej Nowacki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jędrzej Nowacki 2010-04-26 04:12:15 PDT
==11624==    at 0x4024C4C: malloc (vg_replace_malloc.c:195)
==11624==    by 0x40F98CB: WTF::fastMalloc(unsigned int) (FastMalloc.cpp:249)
==11624==    by 0x4053ED7: WTF::FastAllocBase::operator new(unsigned int) (FastAllocBase.h:96)
==11624==    by 0x406E59D: OpaqueJSString::create(JSC::UString const&) (OpaqueJSString.cpp:38)
==11624==    by 0x406CB82: JSValueToStringCopy (JSValueRef.cpp:285)
==11624==    by 0x405083D: QScriptValuePrivate::toString() const (qscriptvalue_p.h:460)
==11624==    by 0x404EB57: QScriptValue::toString() const (qscriptvalue.cpp:333)
Comment 1 Jędrzej Nowacki 2010-04-26 04:21:32 PDT
Created attachment 54273 [details]
Fix v1
Comment 2 Eric Seidel (no email) 2010-04-26 14:11:57 PDT
Comment on attachment 54273 [details]
Fix v1

This looks OK, but I think it could be better.

1.  We should probably have QScriptConverter handle this correctly.  i.e. have a toString() method which takes a value() and context() directly, no?

2.  Isn't there a RetainPtr to use here so that you never have to release the string explicitly?  Then it would look something like this:

RetainPtr<JSStringRef> string = JSValueToStringCopy();
return QScriptConverter::toString(string.get());
Comment 3 Jędrzej Nowacki 2010-05-03 03:45:19 PDT
Created attachment 54912 [details]
Fix v2

(In reply to comment #2)
Thanks for review!
> (From update of attachment 54273 [details])
> This looks OK, but I think it could be better.
> 
> 1.  We should probably have QScriptConverter handle this correctly.  i.e. have
> a toString() method which takes a value() and context() directly, no?
I don't think that it is a good idea or at least not for now. 
 - For simplicity, QtScript ignores exceptions, but it is a temporary solution, they will be exposed trough the QScriptEngine API. Methods of QScriptConverter are not good enough, for exception handling. Moreover for exception handling we will need QScriptEnginePrivate pointer, JSContextRef won't be sufficient, so it means that QScriptConverter couldn't be inlined inside QScriptEnginePrivate.
 - I think that context-less part of the QScriptConverter API should be moved to JSC C API as a generic JavaScript API (toArrayIndex(), toString(double)...)
 - For conversion, I like a function that takes argument and converts it without taking an additional factor (like context). But of course it is a matter taste.
I'm going to reconsider/rethink QScriptConverter idea on exception handling implementation stage.

> 2.  Isn't there a RetainPtr to use here so that you never have to release the
> string explicitly?  Then it would look something like this:
> 
> RetainPtr<JSStringRef> string = JSValueToStringCopy();
> return QScriptConverter::toString(string.get());
Good hint :-). I changed the path to use RetainPtr.
Comment 4 WebKit Commit Bot 2010-05-04 06:54:07 PDT
Comment on attachment 54912 [details]
Fix v2

Clearing flags on attachment: 54912

Committed r58755: <http://trac.webkit.org/changeset/58755>
Comment 5 WebKit Commit Bot 2010-05-04 06:54:13 PDT
All reviewed patches have been landed.  Closing bug.