Bug 33943 - Use fastStrDup instead of strdup
Summary: Use fastStrDup instead of strdup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 33937
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-21 02:44 PST by Kwang Yul Seo
Modified: 2010-02-08 10:22 PST (History)
6 users (show)

See Also:


Attachments
Use fastStrDup instead of strdup (8.87 KB, patch)
2010-01-21 02:55 PST, Kwang Yul Seo
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff
Use fastStrDup (8.88 KB, patch)
2010-01-22 05:47 PST, Kwang Yul Seo
eric: review-
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch (8.02 KB, patch)
2010-02-03 07:39 PST, Kwang Yul Seo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kwang Yul Seo 2010-01-21 02:44:40 PST
fastStrDup is added to FastMalloc in https://bugs.webkit.org/show_bug.cgi?id=33937

Replace strdup/free with fastStrDup/fastFree.
Comment 1 Kwang Yul Seo 2010-01-21 02:55:14 PST
Created attachment 47107 [details]
Use fastStrDup instead of strdup
Comment 2 WebKit Review Bot 2010-01-21 03:00:41 PST
Attachment 47107 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/202872
Comment 3 WebKit Review Bot 2010-01-21 03:00:51 PST
Attachment 47107 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/203032
Comment 4 Kwang Yul Seo 2010-01-21 03:02:55 PST
The following files still contain strdup after this patch.

 - bindings/v8/npruntime.cpp
 - bridge/npruntime.cpp
 - bindings/v8/V8NPUtils.cpp


NPN_UTF8FromIdentifier uses strdup to duplicate a string, so the returned memory must be freed with NPN_MemFree. As NPN_MemFree is implemented with free, strdup can't be replaced with fastStrDup.

Technically speaking, we can change NPN_MemAlloc/NPN_MemFree to use fastMalloc/fastFree internally and use fastStrDup in  NPN_UTF8FromIdentifier. However, some plugins use free instead of NPN_MemFree, replacing strdup with fastStrDup breaks those plugins.

NPN_ReleaseVariantValue has the same issue as NPN_UTF8FromIdentifier.
Comment 5 WebKit Review Bot 2010-01-21 03:03:06 PST
Attachment 47107 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/202876
Comment 6 Kwang Yul Seo 2010-01-21 03:07:10 PST
Build tests won't pass until 33937 is committed.
Comment 7 Darin Adler 2010-01-21 10:01:15 PST
Comment on attachment 47107 [details]
Use fastStrDup instead of strdup

> Index: WebCore/bridge/jni/JNIBridge.cpp
> ===================================================================
> --- WebCore/bridge/jni/JNIBridge.cpp	(revision 53619)
> +++ WebCore/bridge/jni/JNIBridge.cpp	(working copy)
> @@ -301,7 +301,7 @@ JavaMethod::JavaMethod(JNIEnv* env, jobj
>  JavaMethod::~JavaMethod()
>  {
>      if (m_signature)
> -        free(m_signature);
> +        fastFree(m_signature);
>      delete[] m_parameters;
>  };

We could change this to be an OwnPtr now. Also, there is never a need for a null check on a fastFree call.

> @@ -311,7 +311,7 @@ static void appendClassName(StringBuilde
>  {
>      ASSERT(JSLock::lockCount() > 0);
>  
> -    char* c = strdup(className);
> +    char* c = fastStrDup(className);

Unfortunate for performance that this is always on the heap. It would be better if this used Vector<char, 64> or something like that.

>  JavaArray::~JavaArray()
>  {
> -    free(const_cast<char*>(m_type));
> +    fastFree(const_cast<char*>(m_type));
>  }

We could change this to be an OwnPtr now.

> @@ -409,7 +409,7 @@ void JavaArray::setValueAt(ExecState* ex
>          // The type of the array will be something like:
>          // "[Ljava.lang.string;".  This is guaranteed, so no need
>          // for extra sanity checks.
> -        javaClassName = strdup(&m_type[2]);
> +        javaClassName = fastStrDup(&m_type[2]);
>          javaClassName[strchr(javaClassName, ';')-javaClassName] = 0;
>      }
>      jvalue aJValue = convertValueToJValue(exec, aValue, arrayType, javaClassName);
> @@ -472,7 +472,7 @@ void JavaArray::setValueAt(ExecState* ex
>      }
>  
>      if (javaClassName)
> -        free(const_cast<char*>(javaClassName));
> +        fastFree(const_cast<char*>(javaClassName));
>  }

We could change this to be an OwnPtr now.

>  JavaClass::~JavaClass()
>  {
> -    free(const_cast<char*>(m_name));
> +    fastFree(const_cast<char*>(m_name));

We could change this to be an OwnPtr now.

> -    free(m_url);
> +    fastFree(m_url);

We could change this to be an OwnPtr now.

> Index: WebCore/platform/network/curl/ResourceHandleManager.cpp
> ===================================================================
> --- WebCore/platform/network/curl/ResourceHandleManager.cpp	(revision 53619)
> +++ WebCore/platform/network/curl/ResourceHandleManager.cpp	(working copy)
> @@ -135,13 +135,13 @@ ResourceHandleManager::~ResourceHandleMa
>      curl_multi_cleanup(m_curlMultiHandle);
>      curl_share_cleanup(m_curlShareHandle);
>      if (m_cookieJarFileName)
> -        free(m_cookieJarFileName);
> +        fastFree(m_cookieJarFileName);
>      curl_global_cleanup();
>  }

We could change this to be an OwnPtr now.

>      // url is in ASCII so latin1() will only convert it to char* without character translation.
> -    d->m_url = strdup(url.latin1().data());
> +    d->m_url = fastStrDup(url.latin1().data());
>      curl_easy_setopt(d->m_handle, CURLOPT_URL, d->m_url);

I don't see the change to the free call here. Is this introducing a mismatched malloc/free pair?

review- because the try bots say this breaks the build
Comment 8 Kwang Yul Seo 2010-01-21 14:57:26 PST
d->m_url is freed in ResourceHandleInternal::~ResourceHandleInternal, so there is no mismatched malloc/free pair. We could change this to be an OwnPtr too.

I will update the patch as you suggested.
Comment 9 Darin Adler 2010-01-21 23:17:38 PST
All that stuff I said about OwnPtr is wrong. OwnPtr does a delete, not a free. So if we wanted to use a smart pointer we'd either need to use a new one or use OwnArrayPtr and use something like strDup that did new char[] instead of fastMalloc. Oof. The strdup function isn't so great.
Comment 10 Kwang Yul Seo 2010-01-22 05:47:53 PST
Created attachment 47194 [details]
Use fastStrDup

fastStrDup is not so great as we can't use OwnPtr. However, it is still valuable because we can use fastFree everywhere. In addition to it, some compilers without strdup can benefit from fastStrDup.

I submit the patch again to make it pass build tests as 33943 is now committed in the trunk.
Comment 11 Kwang Yul Seo 2010-01-22 05:49:21 PST
I will file a separate bug for using Vector<char, 64> to avoid a heap allocation.
Comment 12 WebKit Commit Bot 2010-01-22 11:19:01 PST
Comment on attachment 47194 [details]
Use fastStrDup

Rejecting patch 47194 from commit-queue.

Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1
Last 500 characters of output:
C8Bindings10JavaMethod9signatureEv in JNIBridge.o
      __ZN3JSC8Bindings9JavaArrayC1EP8_jobjectPKcN3WTF10PassRefPtrINS0_10RootObjectEEE in JNIBridge.o
      __ZN3JSC8Bindings9JavaArrayC2EP8_jobjectPKcN3WTF10PassRefPtrINS0_10RootObjectEEE in JNIBridge.o
ld: symbol(s) not found
collect2: ld returned 1 exit status
** BUILD FAILED **

The following build commands failed:
WebCore:
	Ld /Users/eseidel/Projects/CommitQueue/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore normal i386
(1 failure)


Full output: http://webkit-commit-queue.appspot.com/results/204912
Comment 13 Eric Seidel (no email) 2010-02-02 14:00:05 PST
Comment on attachment 47194 [details]
Use fastStrDup

Looks like this would break the mac build.  r-
Comment 14 Kwang Yul Seo 2010-02-03 07:39:16 PST
Created attachment 48030 [details]
Patch

Update the patch. The build failed because WTF::fastStrDup is not exported. I filed a separate bug for this in https://bugs.webkit.org/show_bug.cgi?id=34526
Comment 15 Kwang Yul Seo 2010-02-08 08:44:38 PST
As bug 34526 is now landed, this needs to reviewed again.
Comment 16 WebKit Commit Bot 2010-02-08 10:22:06 PST
Comment on attachment 48030 [details]
Patch

Clearing flags on attachment: 48030

Committed r54495: <http://trac.webkit.org/changeset/54495>
Comment 17 WebKit Commit Bot 2010-02-08 10:22:14 PST
All reviewed patches have been landed.  Closing bug.