186878 – JSImmutableButterfly can't be allocated from a subspace with HeapCell::Kind::Auxiliary

Bug 186878 - JSImmutableButterfly can't be allocated from a subspace with HeapCell::Kind::Auxiliary

Summary: JSImmutableButterfly can't be allocated from a subspace with HeapCell::Kind::...

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	JavaScriptCore (show other bugs)
Version:	WebKit Nightly Build
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Saam Barati

URL:
Keywords:	InRadar

Depends on:	187059
Blocks:
	Show dependency tree / graph

Reported:	2018-06-20 23:39 PDT by Saam Barati
Modified:	2018-06-26 18:08 PDT (History)
CC List:	17 users (show)

See Also:	186215

Attachments
WIP (4.03 KB, patch) 2018-06-21 12:35 PDT, Saam Barati	ews-watchlist: commit-queue-	Details \| Formatted Diff \| Diff
Archive of layout-test-results from ews100 for mac-sierra (2.41 MB, application/zip) 2018-06-21 13:39 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.98 MB, application/zip) 2018-06-21 13:42 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews117 for mac-sierra (3.06 MB, application/zip) 2018-06-21 14:24 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (8.39 MB, application/zip) 2018-06-21 14:30 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews205 for win-future (12.78 MB, application/zip) 2018-06-21 14:42 PDT, EWS Watchlist	no flags	Details
patch (7.42 KB, patch) 2018-06-22 14:30 PDT, Saam Barati	mark.lam: review+ ews-watchlist: commit-queue-	Details \| Formatted Diff \| Diff
Archive of layout-test-results from ews104 for mac-sierra-wk2 (2.81 MB, application/zip) 2018-06-22 16:03 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews102 for mac-sierra (2.29 MB, application/zip) 2018-06-22 16:04 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews117 for mac-sierra (3.04 MB, application/zip) 2018-06-22 16:07 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.34 MB, application/zip) 2018-06-22 16:22 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews200 for win-future (12.95 MB, application/zip) 2018-06-22 18:43 PDT, EWS Watchlist	no flags	Details
patch for landing if EWS passes (9.28 KB, patch) 2018-06-23 16:07 PDT, Saam Barati	ews-watchlist: commit-queue-	Details \| Formatted Diff \| Diff
Archive of layout-test-results from ews114 for mac-sierra (3.02 MB, application/zip) 2018-06-23 18:45 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews205 for win-future (12.84 MB, application/zip) 2018-06-23 19:54 PDT, EWS Watchlist	no flags	Details
patch for landing if EWS passes #2 (14.53 KB, patch) 2018-06-24 20:01 PDT, Saam Barati	ews-watchlist: commit-queue-	Details \| Formatted Diff \| Diff
Archive of layout-test-results from ews205 for win-future (12.78 MB, application/zip) 2018-06-24 21:55 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.77 MB, application/zip) 2018-06-25 00:06 PDT, EWS Watchlist	no flags	Details
patch for landing attempt #3 (15.54 KB, patch) 2018-06-25 12:09 PDT, Saam Barati	no flags	Details \| Formatted Diff \| Diff
patch for landing (17.50 KB, patch) 2018-06-25 15:06 PDT, Saam Barati	no flags	Details \| Formatted Diff \| Diff
patch for landing (17.90 KB, patch) 2018-06-25 16:16 PDT, Saam Barati	no flags	Details \| Formatted Diff \| Diff
WIP (26.88 KB, patch) 2018-06-26 15:10 PDT, Saam Barati	no flags	Details \| Formatted Diff \| Diff
patch (30.96 KB, patch) 2018-06-26 16:01 PDT, Saam Barati	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (22) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Saam Barati 2018-06-20 23:39:43 PDT

This breaks our conservative marking of the object. Check out this code:


    HeapUtil::findGCObjectPointersForMarking(
        m_heap, markingVersion, newlyAllocatedVersion, filter, p,
        [&] (void* p, HeapCell::Kind cellKind) {
            if (cellKind == HeapCell::JSCell)
                markHook.markKnownJSCell(static_cast<JSCell*>(p));
            
            if (m_size == m_capacity)
                grow();
            
            m_roots[m_size++] = bitwise_cast<HeapCell*>(p);
        });


I think I'll also need to extend findGCObjectPointersForMarking a bit. It does some stuff conditionally on Auxiliary for butterflies, but here, we'll have a kind of Cell for JSImmutableButterfly. I wonder if we should change up how we use JSImmutableButterfly to act more like other butterflies, and obviate this having to be different than other butterflies.

Comment 1 Saam Barati 2018-06-20 23:40:54 PDT

<rdar://problem/40568659>

Comment 2 Saam Barati 2018-06-21 12:35:27 PDT

Created attachment 343258 [details]
WIP

Going to run some memory tests before landing this.

Comment 3 EWS Watchlist 2018-06-21 12:48:35 PDT

Attachment 343258 [details] did not pass style-queue:


ERROR: Source/WebCore/page/FrameView.cpp:4134:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 4 EWS Watchlist 2018-06-21 13:39:47 PDT

Comment on attachment 343258 [details]
WIP

Attachment 343258 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8280282

New failing tests:
editing/selection/navigation-clears-editor-state.html
performance-api/performance-observer-no-document-leak.html
fast/misc/test-observegc.html

Comment 5 EWS Watchlist 2018-06-21 13:39:49 PDT

Created attachment 343266 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

Comment 6 EWS Watchlist 2018-06-21 13:42:17 PDT

Comment on attachment 343258 [details]
WIP

Attachment 343258 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8280261

New failing tests:
editing/selection/navigation-clears-editor-state.html
performance-api/performance-observer-no-document-leak.html

Comment 7 EWS Watchlist 2018-06-21 13:42:19 PDT

Created attachment 343267 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

Comment 8 EWS Watchlist 2018-06-21 14:24:58 PDT

Comment on attachment 343258 [details]
WIP

Attachment 343258 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8280606

New failing tests:
editing/selection/navigation-clears-editor-state.html
performance-api/performance-observer-no-document-leak.html
plugins/refcount-leaks.html
fast/misc/test-observegc.html

Comment 9 EWS Watchlist 2018-06-21 14:24:59 PDT

Created attachment 343274 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

Comment 10 EWS Watchlist 2018-06-21 14:30:18 PDT

Comment on attachment 343258 [details]
WIP

Attachment 343258 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8280520

New failing tests:
editing/selection/navigation-clears-editor-state.html
performance-api/performance-observer-no-document-leak.html
fast/misc/test-observegc.html

Comment 11 EWS Watchlist 2018-06-21 14:30:20 PDT

Created attachment 343275 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

Comment 12 EWS Watchlist 2018-06-21 14:41:57 PDT

Comment on attachment 343258 [details]
WIP

Attachment 343258 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8281106

New failing tests:
fast/dom/reference-cycle-leaks.html
performance-api/performance-observer-no-document-leak.html
fast/misc/test-observegc.html

Comment 13 EWS Watchlist 2018-06-21 14:42:09 PDT

Created attachment 343277 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

Comment 14 Saam Barati 2018-06-21 14:56:42 PDT

Seems like this is causing tests to fail that measure how many objects are left around. So it's probably an indication that the more conservative scan is keeping more around. I'm not sure how much we care about these layout test results. I'm going to look more at these tests. They seem like they are already prone to being flaky since some assert things about GC collecting objects.

Comment 15 Saam Barati 2018-06-22 14:30:08 PDT

Created attachment 343368 [details]
patch

Comment 16 Mark Lam 2018-06-22 15:16:43 PDT

Comment on attachment 343368 [details]
patch

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

r=me

> Source/JavaScriptCore/ChangeLog:14
> +        are Auxiliary. This means that if the stack were the only thing pointing to a

/the stack were/the stack is/.  "was" also works.

> Source/JavaScriptCore/heap/HeapUtil.h:107
> +                func(pointer, candidate->handle().cellKind());

Why re-compute the cellKind() every time here?  The candidate doesn't change.  Hence, the cellKind() shouldn't change either, no?

Oh, I see, cellKind() is only used in here.  I noticed that candidate->handle() is computed and used multiple times.  Can you pre-compute and cache it above instead?

Comment 17 Mark Lam 2018-06-22 15:18:56 PDT

Comment on attachment 343368 [details]
patch

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

>> Source/JavaScriptCore/heap/HeapUtil.h:107
>> +                func(pointer, candidate->handle().cellKind());
> 
> Why re-compute the cellKind() every time here?  The candidate doesn't change.  Hence, the cellKind() shouldn't change either, no?
> 
> Oh, I see, cellKind() is only used in here.  I noticed that candidate->handle() is computed and used multiple times.  Can you pre-compute and cache it above instead?

nit: Oh wait, we still use this lambda in more than one place.  I think we can still precache the cellKind.

Comment 18 EWS Watchlist 2018-06-22 16:03:52 PDT

Comment on attachment 343368 [details]
patch

Attachment 343368 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8296440

New failing tests:
editing/selection/navigation-clears-editor-state.html

Comment 19 EWS Watchlist 2018-06-22 16:03:54 PDT

Created attachment 343385 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

Comment 20 EWS Watchlist 2018-06-22 16:04:01 PDT

Comment on attachment 343368 [details]
patch

Attachment 343368 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8296513

New failing tests:
editing/selection/navigation-clears-editor-state.html

Comment 21 EWS Watchlist 2018-06-22 16:04:03 PDT

Created attachment 343386 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

Comment 22 EWS Watchlist 2018-06-22 16:07:34 PDT

Comment on attachment 343368 [details]
patch

Attachment 343368 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8296256

New failing tests:
editing/selection/navigation-clears-editor-state.html
plugins/refcount-leaks.html

Comment 23 EWS Watchlist 2018-06-22 16:07:36 PDT

Created attachment 343388 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

Comment 24 Saam Barati 2018-06-22 16:13:09 PDT

(In reply to Build Bot from comment #20)
> Comment on attachment 343368 [details]
> patch
> 
> Attachment 343368 [details] did not pass mac-ews (mac):
> Output: https://webkit-queues.webkit.org/results/8296513
> 
> New failing tests:
> editing/selection/navigation-clears-editor-state.html

These tests are vulnerable to conservative roots skewing them

Comment 25 EWS Watchlist 2018-06-22 16:22:42 PDT

Comment on attachment 343368 [details]
patch

Attachment 343368 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8296375

New failing tests:
editing/selection/navigation-clears-editor-state.html

Comment 26 EWS Watchlist 2018-06-22 16:22:44 PDT

Created attachment 343391 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

Comment 27 EWS Watchlist 2018-06-22 18:43:06 PDT

Comment on attachment 343368 [details]
patch

Attachment 343368 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8299011

New failing tests:
fast/dom/reference-cycle-leaks.html

Comment 28 EWS Watchlist 2018-06-22 18:43:17 PDT

Created attachment 343412 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

Comment 29 Saam Barati 2018-06-23 16:07:36 PDT

Created attachment 343457 [details]
patch for landing if EWS passes

Comment 30 EWS Watchlist 2018-06-23 18:45:46 PDT

Comment on attachment 343457 [details]
patch for landing if EWS passes

Attachment 343457 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8308669

New failing tests:
plugins/refcount-leaks.html

Comment 31 EWS Watchlist 2018-06-23 18:45:48 PDT

Created attachment 343461 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6

Comment 32 EWS Watchlist 2018-06-23 19:54:47 PDT

Comment on attachment 343457 [details]
patch for landing if EWS passes

Attachment 343457 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8309118

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html

Comment 33 EWS Watchlist 2018-06-23 19:54:59 PDT

Created attachment 343464 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

Comment 34 Saam Barati 2018-06-24 20:01:03 PDT

Created attachment 343477 [details]
patch for landing if EWS passes #2

Comment 35 EWS Watchlist 2018-06-24 21:55:37 PDT

Comment on attachment 343477 [details]
patch for landing if EWS passes #2

Attachment 343477 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8320540

New failing tests:
http/tests/security/video-poster-cross-origin-crash2.html

Comment 36 EWS Watchlist 2018-06-24 21:55:49 PDT

Created attachment 343484 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

Comment 37 EWS Watchlist 2018-06-25 00:06:47 PDT

Comment on attachment 343477 [details]
patch for landing if EWS passes #2

Attachment 343477 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8325913

New failing tests:
plugins/refcount-leaks.html

Comment 38 EWS Watchlist 2018-06-25 00:06:49 PDT

Created attachment 343489 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

Comment 39 Saam Barati 2018-06-25 12:09:06 PDT

Created attachment 343523 [details]
patch for landing attempt #3

Comment 40 Saam Barati 2018-06-25 15:06:18 PDT

Created attachment 343546 [details]
patch for landing

Comment 41 Ryosuke Niwa 2018-06-25 15:18:06 PDT

Comment on attachment 343546 [details]
patch for landing

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

> LayoutTests/editing/selection/navigation-clears-editor-state.html:62
> +            if (delta > 60)

Maybe we should do count * 0.3 instead?

Comment 42 Ryosuke Niwa 2018-06-25 15:27:29 PDT

(In reply to Ryosuke Niwa from comment #41)
> Comment on attachment 343546 [details]
> patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343546&action=review
> 
> > LayoutTests/editing/selection/navigation-clears-editor-state.html:62
> > +            if (delta > 60)
> 
> Maybe we should do count * 0.3 instead?

Otherwise the change to this test looks good.

Comment 43 Saam Barati 2018-06-25 16:13:41 PDT

(In reply to Ryosuke Niwa from comment #42)
> (In reply to Ryosuke Niwa from comment #41)
> > Comment on attachment 343546 [details]
> > patch for landing
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=343546&action=review
> > 
> > > LayoutTests/editing/selection/navigation-clears-editor-state.html:62
> > > +            if (delta > 60)
> > 
> > Maybe we should do count * 0.3 instead?
> 
> Otherwise the change to this test looks good.

Sounds good. I'll make this change. I'm also going to bump the timeout up a bit.

Comment 44 Saam Barati 2018-06-25 16:16:32 PDT

Created attachment 343552 [details]
patch for landing

Comment 45 WebKit Commit Bot 2018-06-25 16:56:41 PDT

Comment on attachment 343552 [details]
patch for landing

Clearing flags on attachment: 343552

Committed r233184: <https://trac.webkit.org/changeset/233184>

Comment 46 WebKit Commit Bot 2018-06-25 16:56:44 PDT

All reviewed patches have been landed.  Closing bug.

Comment 47 WebKit Commit Bot 2018-06-26 12:01:54 PDT

Re-opened since this is blocked by bug 187059

Comment 48 Saam Barati 2018-06-26 12:30:18 PDT

Seems like I need to use our other approach of introducing a new HeapCell::Kind called something like JSCellWithInteriorPointers

Comment 49 Saam Barati 2018-06-26 15:10:36 PDT

Created attachment 343645 [details]
WIP

want to test it on EWS

Comment 50 Saam Barati 2018-06-26 16:01:55 PDT

Created attachment 343653 [details]
patch

Comment 51 WebKit Commit Bot 2018-06-26 18:08:32 PDT

Comment on attachment 343653 [details]
patch

Clearing flags on attachment: 343653

Committed r233236: <https://trac.webkit.org/changeset/233236>

Comment 52 WebKit Commit Bot 2018-06-26 18:08:34 PDT

All reviewed patches have been landed.  Closing bug.