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.
<rdar://problem/27462104>
Created attachment 294663 [details] proposed patch. Still need to run JSC tests locally.
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?
JSC tests pass. I will upload a new patch that passes StringView (instead of StringView&) in JSGlobalObjectFunction.cpp.
Created attachment 294680 [details] updated patch.
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.
(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.
(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.
Thanks for the review. Landed in r208699: <http://trac.webkit.org/r208699>.
(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.