Bug 164701 – Some of JSStringView::SafeView methods are not idiomatically safe for JSString to StringView conversions.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Specifically, characters8(), characters16(), and operator[] converts the JSString to a StringView via get(), and then uses the StringView without first checking if an exception was thrown during the conversion.  Instead, we should remove these 3 convenience methods, and make the caller explicitly call get() and do the appropriate exception checks before using the StringView.

[Mark Lam]

<rdar://problem/27462104>

[Mark Lam]

Created attachment 294663 [details]
proposed patch.

Still need to run JSC tests locally.

[Darin Adler]

Comment on attachment 294663 [details]
proposed patch.

StringView& seems like a strange type. Why pass in a reference to a StringView instead of passing in a StringView?

[Mark Lam]

JSC tests pass.  I will upload a new patch that passes StringView (instead of StringView&) in JSGlobalObjectFunction.cpp.

[Mark Lam]

Created attachment 294680 [details]
updated patch.

[Darin Adler]

Comment on attachment 294680 [details]
updated patch.

I don’t think the SafeView class works at all now, since get() can raise an exception; hiding the ExecState inside the SafeView object just makes things even more confusing. It’s not at all obvious that a function named get() with no arguments can set an exception. We should change call sites to use unsafeView() directly and remove the view() function and SafeView entirely.

[Darin Adler]

(In reply to comment #6)
> I don’t think the SafeView class works at all now, since get() can raise an
> exception; hiding the ExecState inside the SafeView object just makes things
> even more confusing. It’s not at all obvious that a function named get()
> with no arguments can set an exception. We should change call sites to use
> unsafeView() directly and remove the view() function and SafeView entirely.

I suppose SafeView does do one thing; keeps the JSString on the stack even when it’s not being used any more.

[Mark Lam]

(In reply to comment #7)
> (In reply to comment #6)
> > I don’t think the SafeView class works at all now, since get() can raise an
> > exception; hiding the ExecState inside the SafeView object just makes things
> > even more confusing. It’s not at all obvious that a function named get()
> > with no arguments can set an exception. We should change call sites to use
> > unsafeView() directly and remove the view() function and SafeView entirely.
> 
> I suppose SafeView does do one thing; keeps the JSString on the stack even
> when it’s not being used any more.

I'll land this patch for now to fix the bug, and look into removing the use of SafeView in a subsequent patch.

[Mark Lam]

Thanks for the review.  Landed in r208699: <http://trac.webkit.org/r208699>.

[Mark Lam]

(In reply to comment #8)
> I'll land this patch for now to fix the bug, and look into removing the use
> of SafeView in a subsequent patch.

Will replace the use of SafeView in https://bugs.webkit.org/show_bug.cgi?id=164777.