Bug 168567 - CachedCall should let GC know to keep its arguments alive.
Summary: CachedCall should let GC know to keep its arguments alive.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on: 168609
Blocks:
  Show dependency treegraph
 
Reported: 2017-02-18 19:15 PST by Mark Lam
Modified: 2017-02-20 17:52 PST (History)
12 users (show)

See Also:


Attachments
proposed patch. (14.22 KB, patch)
2017-02-18 20:56 PST, Mark Lam
mark.lam: review-
Details | Formatted Diff | Diff
proposed patch: improved and simpler. (13.50 KB, patch)
2017-02-19 00:02 PST, Mark Lam
mark.lam: review-
Details | Formatted Diff | Diff
proposed patch. (17.71 KB, patch)
2017-02-19 08:13 PST, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (22.30 KB, patch)
2017-02-19 09:36 PST, Mark Lam
sbarati: review+
Details | Formatted Diff | Diff
Patch for landing. (22.07 KB, patch)
2017-02-19 16:18 PST, Mark Lam
no flags Details | Formatted Diff | Diff
patch for re-landing. (23.56 KB, patch)
2017-02-20 15:01 PST, Mark Lam
no flags Details | Formatted Diff | Diff
patch for re-landing: with a slightly improved ChangeLog comment. (23.60 KB, patch)
2017-02-20 15:07 PST, 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 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.