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

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

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.

[Saam Barati]

<rdar://problem/40568659>

[Saam Barati]

Created attachment 343258 [details]
WIP

Going to run some memory tests before landing this.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Saam Barati]

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.

[Saam Barati]

Created attachment 343368 [details]
patch

[Mark Lam]

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?

[Mark Lam]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Saam Barati]

(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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Saam Barati]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Saam Barati]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Saam Barati]

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

[Saam Barati]

Created attachment 343546 [details]
patch for landing

[Ryosuke Niwa]

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?

[Ryosuke Niwa]

(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.

[Saam Barati]

(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.

[Saam Barati]

Created attachment 343552 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 343552 [details]
patch for landing

Clearing flags on attachment: 343552

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

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 187059

[Saam Barati]

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

[Saam Barati]

Created attachment 343645 [details]
WIP

want to test it on EWS

[Saam Barati]

Created attachment 343653 [details]
patch

[WebKit Commit Bot]

Comment on attachment 343653 [details]
patch

Clearing flags on attachment: 343653

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

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.