Bug 145870 - Use NakedPtr<Exception>& to return exception results.
Summary: Use NakedPtr<Exception>& to return exception results.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-06-10 20:55 PDT by Mark Lam
Modified: 2015-06-16 13:53 PDT (History)
9 users (show)

See Also:


Attachments
candidate patch. (46.46 KB, patch)
2015-06-10 21:50 PDT, Mark Lam
rniwa: review+
Details | Formatted Diff | Diff
patch 2: using NakedPtr<Exception>. (56.59 KB, patch)
2015-06-12 23:08 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
patch 3: applied Anders' feedback. (56.06 KB, patch)
2015-06-16 13:12 PDT, Mark Lam
fpizlo: review+
Details | Formatted Diff | Diff
patch for landing. (56.22 KB, patch)
2015-06-16 13:42 PDT, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2015-06-10 20:55:01 PDT
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.
Comment 1 Mark Lam 2015-06-10 21:50:17 PDT
Created attachment 254706 [details]
candidate patch.
Comment 2 Ryosuke Niwa 2015-06-11 12:38:47 PDT
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.
Comment 3 Mark Lam 2015-06-12 23:06:53 PDT
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).
Comment 4 Mark Lam 2015-06-12 23:08:13 PDT
Created attachment 254851 [details]
patch 2: using NakedPtr<Exception>.
Comment 5 Mark Lam 2015-06-12 23:15:20 PDT
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.
Comment 6 Anders Carlsson 2015-06-16 11:45:00 PDT
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.
Comment 7 Mark Lam 2015-06-16 13:12:04 PDT
Created attachment 254962 [details]
patch 3: applied Anders' feedback.

Thanks for the feedback.  The patch has been updated.
Comment 8 Filip Pizlo 2015-06-16 13:16:50 PDT
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?
Comment 9 Filip Pizlo 2015-06-16 13:25:23 PDT
(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?
Comment 10 Anders Carlsson 2015-06-16 13:35:35 PDT
(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.
Comment 11 Filip Pizlo 2015-06-16 13:37:53 PDT
(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?
Comment 12 Darin Adler 2015-06-16 13:41:44 PDT
(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.
Comment 13 Mark Lam 2015-06-16 13:42:09 PDT
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.
Comment 14 Mark Lam 2015-06-16 13:53:03 PDT
Thanks for the reviews.  Landed in r185608: <http://trac.webkit.org/r185608>.