fastStrDup is added to FastMalloc in https://bugs.webkit.org/show_bug.cgi?id=33937 Replace strdup/free with fastStrDup/fastFree.
Created attachment 47107 [details] Use fastStrDup instead of strdup
Attachment 47107 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/202872
Attachment 47107 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/203032
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.
Attachment 47107 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/202876
Build tests won't pass until 33937 is committed.
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
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.
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.
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.
I will file a separate bug for using Vector<char, 64> to avoid a heap allocation.
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 on attachment 47194 [details] Use fastStrDup Looks like this would break the mac build. r-
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
As bug 34526 is now landed, this needs to reviewed again.
Comment on attachment 48030 [details] Patch Clearing flags on attachment: 48030 Committed r54495: <http://trac.webkit.org/changeset/54495>
All reviewed patches have been landed. Closing bug.