Bug 189652

Summary: Use OpaqueJSString rather than JSRetainPtr inside WebKit
Product: WebKit Reporter: Darin Adler <darin>
Component: JavaScriptCoreAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch none

Darin Adler
Reported 2018-09-16 09:04:20 PDT
Use OpaqueJSString rather than JSRetainPtr inside WebKit
Attachments
Patch (33.13 KB, patch)
2018-09-16 12:32 PDT, Darin Adler 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Darin Adler
Comment 1 2018-09-16 12:32:11 PDT
Created attachment 349873 [details] Patch
Darin Adler
Comment 2 2018-09-16 12:32:59 PDT
Keith, this patch is a response to a comment you made in my last one about using smart pointers more consistently. I realized that inside WebKit we should use RefPtr and Ref, not JSRetainPtr, for JSStringRef.
Saam Barati
Comment 3 2018-09-17 09:29:13 PDT
Comment on attachment 349873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349873&action=review r=me > Source/JavaScriptCore/API/JSContext.mm:104 > + auto sourceURLJS = OpaqueJSString::create([sourceURL absoluteString]); This removes a null check on sourceURL. Is that intended? > Source/JavaScriptCore/API/JSContext.mm:105 > + JSValueRef result = JSEvaluateScript(m_context, scriptJS.get(), nullptr, sourceURLJS.get(), 0, &exceptionValue); Ignore the above comment (I’m on my phone and can’t edit it). I knew that ObjC message send is safe on null, but I didn’t know it’s guaranteed to also return null
WebKit Commit Bot
Comment 4 2018-09-17 09:55:06 PDT
Comment on attachment 349873 [details] Patch Clearing flags on attachment: 349873 Committed r236066: <https://trac.webkit.org/changeset/236066>
WebKit Commit Bot
Comment 5 2018-09-17 09:55:07 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 6 2018-09-17 09:56:20 PDT
<rdar://problem/44523492>
Darin Adler
Comment 7 2018-09-23 15:15:09 PDT
Looks like I missed the JavaScriptCore/API/glib directory when making this change. The code in there still uses JSRetainPtr, not OpaqueJSString.
Note You need to log in before you can comment on or make changes to this bug.