Bug 168567 – CachedCall should let GC know to keep its arguments alive.

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

[Mark Lam]

Patch coming.

[Mark Lam]

<rdar://problem/30475767>

[Mark Lam]

Created attachment 302066 [details]
proposed patch.

[Mark Lam]

Comment on attachment 302066 [details]
proposed patch.

The assertion I added turned out to be prohibitively expensive and makes debug build tests timeout.  I'm going to re-work the solution so that we don't need the assertion.

[Mark Lam]

Created attachment 302069 [details]
proposed patch: improved and simpler.

[Saam Barati]

Comment on attachment 302069 [details]
proposed patch: improved and simpler.

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

r=me

> Source/JavaScriptCore/interpreter/CachedCall.h:41
> +        WTF_FORBID_HEAP_ALLOCATION;

I'm not a huge fan of this macro given the downsides to enforcement you listed inside the WTF changelog. That said, if you keep it, it's definitely worth also putting it on MarkedArgumentsBuffer

> Source/WTF/wtf/ForbidHeapAllocation.h:37
> +    typedef int __thisIsHereToForceASemicolonAfterThisForbidHeapAllocationMacro

Why is this important?

[Mark Lam]

Comment on attachment 302069 [details]
proposed patch: improved and simpler.

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

Thanks, but I just discovered a logical bug: we need to pre-allocate the MarkedArgumentBuffer's m_buffer before I call prepareForRepeatCall() which caches that buffer.  I will need to upload a patch with that fix shortly.

>> Source/JavaScriptCore/interpreter/CachedCall.h:41
>> +        WTF_FORBID_HEAP_ALLOCATION;
> 
> I'm not a huge fan of this macro given the downsides to enforcement you listed inside the WTF changelog. That said, if you keep it, it's definitely worth also putting it on MarkedArgumentsBuffer

Ideally, we should put this on MarkedArgumentBuffer as well, but the VM is malloc'ing a MarkedArgumentBuffer for its emptyList.  It just so happens to work out because that list is never populated with args.  It's bad form, but beyond the scope of the bug I'm trying to fox.  Will look into fixing that at a later time (since it's benign for now).

>> Source/WTF/wtf/ForbidHeapAllocation.h:37
>> +    typedef int __thisIsHereToForceASemicolonAfterThisForbidHeapAllocationMacro
> 
> Why is this important?

So that we can declare "WTF_FORBID_HEAP_ALLOCATION;" instead of just "WTF_FORBID_HEAP_ALLOCATION" which would look weird in the code that uses this macro.  This is consistent with how WTF_MAKE_FAST_ALLOCATED does it.

[Mark Lam]

Comment on attachment 302069 [details]
proposed patch: improved and simpler.

I have a bug I need to fix (as mentioned previously).

[Mark Lam]

Created attachment 302080 [details]
proposed patch.

I'm still doing some local testing on this, but lets get the EWS on it as well.

[Mark Lam]

(In reply to comment #6)
> >> Source/JavaScriptCore/interpreter/CachedCall.h:41
> >> +        WTF_FORBID_HEAP_ALLOCATION;
> > 
> > I'm not a huge fan of this macro given the downsides to enforcement you listed inside the WTF changelog. That said, if you keep it, it's definitely worth also putting it on MarkedArgumentsBuffer
> 
> Ideally, we should put this on MarkedArgumentBuffer as well, but the VM is
> malloc'ing a MarkedArgumentBuffer for its emptyList.  It just so happens to
> work out because that list is never populated with args.  It's bad form, but
> beyond the scope of the bug I'm trying to fox.  Will look into fixing that
> at a later time (since it's benign for now).

Scratch that.  I figured how to fix this on the cheap.  I'll add it to the patch.

[Mark Lam]

Created attachment 302082 [details]
proposed patch.

Tests are passing.  This is ready for a review.

[Saam Barati]

Comment on attachment 302082 [details]
proposed patch.

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

> Source/JavaScriptCore/runtime/ArgList.cpp:70
> +    if (requestedCapacity != UnspecifiedCapacity) {

This feels like a weird way to expand for the use case of asking for a specific size. Why not just allocate the exact desired capacity?

> Source/JavaScriptCore/runtime/ArgList.cpp:71
> +        int safeRequestedCapacity = Checked<int>(requestedCapacity).unsafeGet();

This does what you want?

> Source/JavaScriptCore/runtime/ArgList.h:106
> +    static const size_t UnspecifiedCapacity = 0;

Same comment about this being a weird way to overload. My suggestion would be to have two versions of expanding that both call into the same after expansion helper.

[Mark Lam]

(In reply to comment #11)
> > Source/JavaScriptCore/runtime/ArgList.cpp:70
> > +    if (requestedCapacity != UnspecifiedCapacity) {
> 
> This feels like a weird way to expand for the use case of asking for a
> specific size. Why not just allocate the exact desired capacity?

Technically, the client can still append more arguments after ensureCapacity() is called, thereby necessitating more expansion.  So, I tried to stay consistent with the original behavior of doubling the sizes from the initial inlined capacity.  That said, in practice, if the client wants to ensureCapacity first, then it usually means that the client already knows what the final needed capacity is.  I'll change the implementation to allocate the exact requested capacity after the needed overflow checks.

> > Source/JavaScriptCore/runtime/ArgList.cpp:71
> > +        int safeRequestedCapacity = Checked<int>(requestedCapacity).unsafeGet();
> 
> This does what you want?

Yes, I'm invoking this constructor that ensure that the requestedCapacity does not overflow an int:

    template <typename U> Checked(U value)
    {
        if (!isInBounds<T>(value))
            this->overflowed();
        m_value = static_cast<T>(value);
    }

... and the default behavior of overflowed() is to crash.

> > Source/JavaScriptCore/runtime/ArgList.h:106
> > +    static const size_t UnspecifiedCapacity = 0;
> 
> Same comment about this being a weird way to overload. My suggestion would
> be to have two versions of expanding that both call into the same after
> expansion helper.

This is a good idea.  I will apply this change.

[Mark Lam]

Created attachment 302098 [details]
Patch for landing.

[Mark Lam]

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 168609

[Mark Lam]

FYI, this patch was rolled out in r212665: <http://trac.webkit.org/changeset/212665>.

I've found an issue in MarkedArgumentBuffer::expandCapacity() that may result in random crashes.  The issue is that MarkedArgumentBuffer::expandCapacity() is copying JSValues from the old buffer to the new buffer, and uses m_capacity as the number of values to copy.  It should actually be using m_size.  As a result, depending on what values are present in the old m_buffer, addMarkSet() may crash on a bad heap pointer value computed from the bad JSValue in the old buffer entries beyond m_size.

I will upload a new patch for landing with the fix applied.

[Mark Lam]

Created attachment 302176 [details]
patch for re-landing.

Let's get some EWS testing on it first.

[Mark Lam]

Created attachment 302177 [details]
patch for re-landing: with a slightly improved ChangeLog comment.

[Mark Lam]

Comment on attachment 302177 [details]
patch for re-landing: with a slightly improved ChangeLog comment.

All tests are passing.  Let's re-land.

[WebKit Commit Bot]

Comment on attachment 302177 [details]
patch for re-landing: with a slightly improved ChangeLog comment.

Clearing flags on attachment: 302177

Committed r212692: <http://trac.webkit.org/changeset/212692>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.