Bug 33943 – Use fastStrDup instead of strdup

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kwang Yul Seo]

fastStrDup is added to FastMalloc in https://bugs.webkit.org/show_bug.cgi?id=33937

Replace strdup/free with fastStrDup/fastFree.

[Kwang Yul Seo]

Created attachment 47107 [details]
Use fastStrDup instead of strdup

[WebKit Review Bot]

Attachment 47107 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/202872

[WebKit Review Bot]

Attachment 47107 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/203032

[Kwang Yul Seo]

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.

[WebKit Review Bot]

Attachment 47107 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/202876

[Kwang Yul Seo]

Build tests won't pass until 33937 is committed.

[Darin Adler]

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

[Kwang Yul Seo]

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.

[Darin Adler]

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.

[Kwang Yul Seo]

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.

[Kwang Yul Seo]

I will file a separate bug for using Vector<char, 64> to avoid a heap allocation.

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

Comment on attachment 47194 [details]
Use fastStrDup

Looks like this would break the mac build.  r-

[Kwang Yul Seo]

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

[Kwang Yul Seo]

As bug 34526 is now landed, this needs to reviewed again.

[WebKit Commit Bot]

Comment on attachment 48030 [details]
Patch

Clearing flags on attachment: 48030

Committed r54495: <http://trac.webkit.org/changeset/54495>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.