Bug 168567

Summary: CachedCall should let GC know to keep its arguments alive.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, commit-queue, dbates, fpizlo, ggaren, jfbastien, keith_miller, msaboff, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 168609    
Bug Blocks:    
Attachments:
Description Flags
proposed patch.
mark.lam: review-
proposed patch: improved and simpler.
mark.lam: review-
proposed patch.
none
proposed patch.
saam: review+
Patch for landing.
none
patch for re-landing.
none
patch for re-landing: with a slightly improved ChangeLog comment. none

Description Mark Lam 2017-02-18 19:15:12 PST
Patch coming.
Comment 1 Mark Lam 2017-02-18 19:18:55 PST
<rdar://problem/30475767>
Comment 2 Mark Lam 2017-02-18 20:56:27 PST
Created attachment 302066 [details]
proposed patch.
Comment 3 Mark Lam 2017-02-18 23:17:40 PST
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.
Comment 4 Mark Lam 2017-02-19 00:02:28 PST
Created attachment 302069 [details]
proposed patch: improved and simpler.
Comment 5 Saam Barati 2017-02-19 00:23:38 PST
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?
Comment 6 Mark Lam 2017-02-19 00:31:44 PST
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.
Comment 7 Mark Lam 2017-02-19 00:32:20 PST
Comment on attachment 302069 [details]
proposed patch: improved and simpler.

I have a bug I need to fix (as mentioned previously).
Comment 8 Mark Lam 2017-02-19 08:13:32 PST
Created attachment 302080 [details]
proposed patch.

I'm still doing some local testing on this, but lets get the EWS on it as well.
Comment 9 Mark Lam 2017-02-19 09:09:47 PST
(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.
Comment 10 Mark Lam 2017-02-19 09:36:06 PST
Created attachment 302082 [details]
proposed patch.

Tests are passing.  This is ready for a review.
Comment 11 Saam Barati 2017-02-19 10:14:29 PST
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.
Comment 12 Mark Lam 2017-02-19 15:33:35 PST
(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.
Comment 13 Mark Lam 2017-02-19 16:18:09 PST
Created attachment 302098 [details]
Patch for landing.
Comment 14 Mark Lam 2017-02-19 17:48:32 PST
Thanks for the reviews.  Landed in r212618: <http://trac.webkit.org/r212618>.
Comment 15 WebKit Commit Bot 2017-02-20 12:30:26 PST
Re-opened since this is blocked by bug 168609
Comment 16 Mark Lam 2017-02-20 14:28:08 PST
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.
Comment 17 Mark Lam 2017-02-20 15:01:39 PST
Created attachment 302176 [details]
patch for re-landing.

Let's get some EWS testing on it first.
Comment 18 Mark Lam 2017-02-20 15:07:37 PST
Created attachment 302177 [details]
patch for re-landing: with a slightly improved ChangeLog comment.
Comment 19 Mark Lam 2017-02-20 17:26:40 PST
Comment on attachment 302177 [details]
patch for re-landing: with a slightly improved ChangeLog comment.

All tests are passing.  Let's re-land.
Comment 20 WebKit Commit Bot 2017-02-20 17:52:50 PST
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>
Comment 21 WebKit Commit Bot 2017-02-20 17:52:57 PST
All reviewed patches have been landed.  Closing bug.