Attachment 360522[details] did not pass style-queue:
ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCCallbackFunction.h"
Total errors found: 1 in 62 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 360534[details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Created attachment 360535[details]
Archive of layout-test-results from ews101 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 360537[details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Created attachment 360544[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
Created attachment 360554[details]
Archive of layout-test-results from ews101 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 360555[details]
Archive of layout-test-results from ews114 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 360556[details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Created attachment 360557[details]
Archive of layout-test-results from ews204 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Created attachment 360558[details]
Archive of layout-test-results from ews125 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Cool! Finally I found that this hits the existing bug!
In VM shutdown, if UnlinkedCodeBlock is destroyed before linked CodeBlock, underlying UnlinkedMetadataTable is already gone when destroying CodeBlock.
Previously, EvalCodeBlock is visited before visiting UnlinkedCodeBlock. At that case, we first destroy MetadataTable, unlinking it from the UnlinkedMetadataTable, and then, UnlinkedCodeBlock is destroyed, which leads to destruction of UnlinkedMetadataTable. This is fine.
But this dynamic IsoSubspace creation churns the implicit order of BlockDirectories filed in ObjectSpace (MarkedSpace). When VM shutdown happens, and UnlinkedCodeBlock is destroyed before EvalCodeBlock, then UnlinkedMetadataTable is destroyed while it is still linked.
We can test this case easily by the following script with the above patch.
$.agent.start(`
for (var i = 0; i < 10000; ++i) {
eval('print(' + i + ')');
}
`);
sleepSeconds(10);
Comment on attachment 361042[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=361042&action=review
r- since I think we can do a fair amount of code de-duplication.
Can we forward the SubSpaceAccess to the VM's getter? Currently, both the subspaceFor and the VM's getter methods have to know about concurrent access. If you make the getter on VM be:
Subspace* webAssemblyFunctionSpace(SubspaceAccess mode)
{
if (m_ webAssemblyFunctionSpace || mode == SubspaceAccess::Concurrent)
return webAssemblyFunctionSpace.get();
return ensureWebAssemblyFunctionSpace();
}
Then all the various subspaceFor can be:
template<typename CellType, SubspaceAccess mode>
static Subspace* subspaceFor(VM& vm)
{
return vm. webAssemblyFunctionSpace(mode);
}
Which makes them a lot simpler and doesn't make the VM code any more complicated since you'll only have one method instead of two.
> Source/JavaScriptCore/runtime/JSCell.h:308
> +inline auto subspaceForConcurrently(VM& vm)
> +{
> + return Type::template subspaceFor<Type, SubspaceAccess::Concurrently>(vm);
Why not have:
template<typename Type>
inline Allocator allocatorForNonVirtualConcurrently(VM& vm, size_t allocationSize, AllocatorForMode mode)
{
if (auto subspace = Type::template subspaceFor<Type, SubspaceAccess::Concurrently>(vm))
return subspace->allocatorForNonVirtual(allocationSize, mode);
return { };
}
Then you wouldn't have to duplicate nearly as much code in the JITs.
> Source/JavaScriptCore/runtime/VM.cpp:1228
> +IsoSubspace& VM::ensureBoundFunctionSpaceSlow()
> +{
> + ASSERT(!boundFunctionSpace);
> + auto space = std::make_unique<IsoSubspace> ISO_SUBSPACE_INIT(heap, cellHeapCellType.get(), JSBoundFunction);
> + WTF::storeStoreFence();
> + boundFunctionSpace = WTFMove(space);
> + return *boundFunctionSpace;
> +}
Nit: Can we make a template function or worse-case a macro that does this? This is a lot of duplicated code. :(
Comment on attachment 361042[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=361042&action=review
Thanks. Make sense, we should reduce the duplication by using macro.
>> Source/JavaScriptCore/runtime/JSCell.h:308
>> + return Type::template subspaceFor<Type, SubspaceAccess::Concurrently>(vm);
>
> Why not have:
>
> template<typename Type>
> inline Allocator allocatorForNonVirtualConcurrently(VM& vm, size_t allocationSize, AllocatorForMode mode)
> {
> if (auto subspace = Type::template subspaceFor<Type, SubspaceAccess::Concurrently>(vm))
> return subspace->allocatorForNonVirtual(allocationSize, mode);
> return { };
> }
>
> Then you wouldn't have to duplicate nearly as much code in the JITs.
Make sense. I've changed them to allocatorForNonVirtualConcurrently. But still some uses Subspace* (not Allocator). For those cases, I use subspaceForConcurrently.
>> Source/JavaScriptCore/runtime/VM.cpp:1228
>> +}
>
> Nit: Can we make a template function or worse-case a macro that does this? This is a lot of duplicated code. :(
Nice. Since this depends on the name like "boundFunctionSpace", I think macro is the easiest and cleanest solution here. Changed.
Comment on attachment 361096[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=361096&action=review
r=me with some nits. Looks good!
> Source/JavaScriptCore/ChangeLog:31
> + can run this IsoSubspace code, the collector is never EndPhase. So this is safe.
Is there anyway to assert that the GC isn't in EndPhase when creating an IsoSubspace? Even if it's racy that might still be helpful. I would only worry about this if it's like a one line change though.
> Source/JavaScriptCore/runtime/VM.cpp:1248
> +
Can we undef DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER_SLOW here?
> Source/JavaScriptCore/runtime/VM.cpp:1263
> +
Ditto on undef.
> Source/JavaScriptCore/runtime/VM.h:406
>
Ditto on undef.
> Source/JavaScriptCore/runtime/VM.h:458
> + DYNAMIC_SPACE_AND_SET_DEFINE_MEMBER(moduleProgramExecutableSpace)
Ditto on undef.
Comment on attachment 361096[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=361096&action=review
Thanks!
>> Source/JavaScriptCore/ChangeLog:31
>> + can run this IsoSubspace code, the collector is never EndPhase. So this is safe.
>
> Is there anyway to assert that the GC isn't in EndPhase when creating an IsoSubspace? Even if it's racy that might still be helpful. I would only worry about this if it's like a one line change though.
Nice, I'm now checking this with `ASSERT(!mayBeGCThread() || m_heap->worldIsStopped());` now to ensure the above thing!
>> Source/JavaScriptCore/runtime/VM.cpp:1248
>> +
>
> Can we undef DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER_SLOW here?
Done.
>> Source/JavaScriptCore/runtime/VM.cpp:1263
>> +
>
> Ditto on undef.
Done.
>> Source/JavaScriptCore/runtime/VM.h:406
>>
>
> Ditto on undef.
Done.
>> Source/JavaScriptCore/runtime/VM.h:458
>> + DYNAMIC_SPACE_AND_SET_DEFINE_MEMBER(moduleProgramExecutableSpace)
>
> Ditto on undef.
Done.
2019-01-29 15:58 PST, Yusuke Suzuki
2019-01-29 16:34 PST, Yusuke Suzuki
2019-01-29 16:36 PST, Yusuke Suzuki
2019-01-29 18:12 PST, EWS Watchlist
2019-01-29 18:26 PST, EWS Watchlist
2019-01-29 19:00 PST, EWS Watchlist
2019-01-29 20:26 PST, EWS Watchlist
2019-01-29 20:45 PST, Yusuke Suzuki
2019-01-29 21:18 PST, Yusuke Suzuki
2019-01-29 22:31 PST, EWS Watchlist
2019-01-29 22:42 PST, EWS Watchlist
2019-01-29 22:49 PST, EWS Watchlist
2019-01-29 23:18 PST, EWS Watchlist
2019-01-29 23:24 PST, EWS Watchlist
2019-02-03 23:33 PST, Yusuke Suzuki
2019-02-04 13:55 PST, Yusuke Suzuki