Bug 145870 – Use NakedPtr<Exception>& to return exception results.

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

[Mark Lam]

Before r185259, calls into the VM takes a JSValue* exceptionResult argument for returning any uncaught exception that may have been thrown while executing JS code.  As a result, clients of the VM APIs will declare a local JSValue exceptionResult which is automatically initialized to a null value (i.e. the empty value, not the JS null value).  With r185259, the call APIs were changes to take an Exception*& exceptionResult instead, and the VM functions are responsible for initializing the exceptionResult to null if no exception is thrown.  This introduces 2 issues:

1. the VM code is vulnerable to modifications that may add early returns before the exceptionResult is nullified.  This can result in the exceptionResult being used without initialization.

2. Previously, a client could technically use the same exceptionResult for more than one calls into the VM.  If an earlier call sets it to a thrown value, the thrown value will stick unless a subsequent call throws a different exception.  With the new Exception*& exceptionResult, the VM calls will always clear the exceptionResult before proceeding.  As a result, the client's exceptionResult will be null after the second call even though the first call saw an exception thrown.  This is a change in expected behavior.

To fix these issues, we'll introduce an ExceptionResult class which embeds and auto-initializes an Exception*.  The VM calls will be reverted to only set the exceptionResult if a new exception is thrown.

[Mark Lam]

Created attachment 254706 [details]
candidate patch.

[Ryosuke Niwa]

Comment on attachment 254706 [details]
candidate patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=254706&action=review

> Source/JavaScriptCore/runtime/ExceptionResult.h:35
> +template <typename T>
> +class Ptr {
> +public:

I think this template will be useful in WTF as well.  e.g. NodeRareData could use this for m_childNodeList.

> Source/JavaScriptCore/runtime/ExceptionResult.h:61
> +class ExceptionResult : public Ptr<Exception> {

I think it's better to call this ExceptionPtr or ExceptionResultPtr so that overloading operators are easier to reason about in callsites.
It seems like we can just use typedef??  Or maybe we should just use Ptr<Exceptoin> directly since Ptr seems useful elsewhere.

[Mark Lam]

Since there is a use for the self initializing pointer class in the rest of the code base, I think it is a good idea to implement the pointer class in WTF.  However, it turns out that we can't use Ptr as the class name because there is a collision with a typedef in MacTypes.h.

In consultation with Anders and Andreas, we settled on NakedPtr as a good name to go with for now.  naked_ptr is one of the alternate names for this concept in the proposal to add this pointer class to the C++ stdlib (see http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3514.pdf).

Will upload an updated patch for review again (since I've made substantial changes to the patch).

[Mark Lam]

Created attachment 254851 [details]
patch 2: using NakedPtr<Exception>.

[Mark Lam]

Comment on attachment 254851 [details]
patch 2: using NakedPtr<Exception>.

View in context: https://bugs.webkit.org/attachment.cgi?id=254851&action=review

> Tools/TestWebKitAPI/TestWebKitAPI.vcxproj/TestWebKitAPI.vcxproj:369
> -</Project>
>  \ No newline at end of file
> +</Project>

Will remove this unnecessary change before landing.

> Tools/TestWebKitAPI/TestWebKitAPI.vcxproj/TestWebKitAPI.vcxproj.filters:181
> -</Project>
>  \ No newline at end of file
> +</Project>

Will remove this unnecessary change before landing.

[Anders Carlsson]

Comment on attachment 254851 [details]
patch 2: using NakedPtr<Exception>.

View in context: https://bugs.webkit.org/attachment.cgi?id=254851&action=review

> Source/WTF/wtf/NakedPtr.h:57
> +    // This conversion operator allows implicit conversion to bool but not to other integer types.
> +    typedef T* (NakedPtr::*UnspecifiedBoolType);
> +    operator UnspecifiedBoolType() const { return m_ptr ? &NakedPtr::m_ptr : nullptr; }

Instead of this, please add an explicit operator bool() const.

[Mark Lam]

Created attachment 254962 [details]
patch 3: applied Anders' feedback.

Thanks for the feedback.  The patch has been updated.

[Filip Pizlo]

Comment on attachment 254962 [details]
patch 3: applied Anders' feedback.

View in context: https://bugs.webkit.org/attachment.cgi?id=254962&action=review

R=me but please consider the two suggestions.

> Source/WTF/wtf/NakedPtr.h:33
> +template <typename T> class NakedPtr {

Please add a comment up here about what this is for.  In particular, that it initializes the pointer to null.

> Source/WTF/wtf/NakedPtr.h:53
> +    bool operator!() const { return !m_ptr; }

Can we use UnspecifiedNullType here, to prevent accidental coercions to null?

[Filip Pizlo]

(In reply to comment #6)
> Comment on attachment 254851 [details]
> patch 2: using NakedPtr<Exception>.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254851&action=review
> 
> > Source/WTF/wtf/NakedPtr.h:57
> > +    // This conversion operator allows implicit conversion to bool but not to other integer types.
> > +    typedef T* (NakedPtr::*UnspecifiedBoolType);
> > +    operator UnspecifiedBoolType() const { return m_ptr ? &NakedPtr::m_ptr : nullptr; }
> 
> Instead of this, please add an explicit operator bool() const.

Oh, I saw that I had the opposite feedback.  Why is explicit operator bool() better?

[Anders Carlsson]

(In reply to comment #8)

> > Source/WTF/wtf/NakedPtr.h:53
> > +    bool operator!() const { return !m_ptr; }
> 
> Can we use UnspecifiedNullType here, to prevent accidental coercions to null?

operator! will only be called when you use the ! operator.

(In reply to comment #9)
> 
> Oh, I saw that I had the opposite feedback.  Why is explicit operator bool()
> better?

I find that it's easier to read while still having the same guarantees agents accidental conversion.

[Filip Pizlo]

(In reply to comment #10)
> (In reply to comment #8)
> 
> > > Source/WTF/wtf/NakedPtr.h:53
> > > +    bool operator!() const { return !m_ptr; }
> > 
> > Can we use UnspecifiedNullType here, to prevent accidental coercions to null?
> 
> operator! will only be called when you use the ! operator.

Sorry, I meant to put that comment on "operator bool()".  By mistake, I put it on operator!().

> 
> (In reply to comment #9)
> > 
> > Oh, I saw that I had the opposite feedback.  Why is explicit operator bool()
> > better?
> 
> I find that it's easier to read while still having the same guarantees
> agents accidental conversion.

Does this mean that we should eliminate all of our uses of UnspecifiedNullType?

[Darin Adler]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > Oh, I saw that I had the opposite feedback.  Why is explicit operator bool()
> > > better?
> > 
> > I find that it's easier to read while still having the same guarantees
> > agents accidental conversion.
> 
> Does this mean that we should eliminate all of our uses of
> UnspecifiedNullType?

I am pretty sure it does. UnspecifiedNullType was a workaround for the lack of the "explicit operator bool()" feature.

[Mark Lam]

Created attachment 254966 [details]
patch for landing.

For the record, in the previous patch, I neglected to add the "explicit" keyword to operator bool().  That has been remedied in this patch.

[Mark Lam]

Thanks for the reviews.  Landed in r185608: <http://trac.webkit.org/r185608>.