Add support for WeakRef
Created attachment 371736 [details] Patch
Comment on attachment 371736 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371736&action=review r=me > Source/JavaScriptCore/runtime/JSWeakObjectRef.cpp:2 > + * Copyright (C) 2015-2016 Apple, Inc. All rights reserved. Update the copyright year. > Source/JavaScriptCore/runtime/WeakRefConstructor.h:30 > +class  à : public InternalFunction { Is there something wrong with the nam or encoding of the name of this class?
Comment on attachment 371736 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371736&action=review >> Source/JavaScriptCore/runtime/WeakRefConstructor.h:30 >> +class  à : public InternalFunction { > > Is there something wrong with the nam or encoding of the name of this class? Woah... I have no idea. I didn’t think C++ supported Unicode... will fix.
Comment on attachment 371736 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371736&action=review > Source/JavaScriptCore/runtime/VM.cpp:1099 > + finalizeSynchronousJSExecution(); Don't you need something like this for the micro task queue in browser?
(In reply to Saam Barati from comment #4) > Comment on attachment 371736 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=371736&action=review > > > Source/JavaScriptCore/runtime/VM.cpp:1099 > > + finalizeSynchronousJSExecution(); > > Don't you need something like this for the micro task queue in browser? E.g, call this function from that micro task queue.
Created attachment 371952 [details] Patch
Comment on attachment 371952 [details] Patch Attachment 371952 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12454780 Number of test failures exceeded the failure limit.
Created attachment 371958 [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
Comment on attachment 371952 [details] Patch Attachment 371952 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12454777 Number of test failures exceeded the failure limit.
Created attachment 371959 [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
Comment on attachment 371952 [details] Patch Attachment 371952 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12454710 Number of test failures exceeded the failure limit.
Created attachment 371960 [details] Archive of layout-test-results from ews115 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 371952 [details] Patch Attachment 371952 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12454674 Number of test failures exceeded the failure limit.
Created attachment 371961 [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.14.5
Comment on attachment 371952 [details] Patch Attachment 371952 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12454927 Number of test failures exceeded the failure limit.
Created attachment 371970 [details] Archive of layout-test-results from ews210 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews210 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Created attachment 372038 [details] Patch
Comment on attachment 372038 [details] Patch Attachment 372038 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12463119 Number of test failures exceeded the failure limit.
Created attachment 372044 [details] Archive of layout-test-results from ews115 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 372046 [details] Patch
Comment on attachment 372046 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372046&action=review r=me for JSC part. > Source/JavaScriptCore/runtime/JSWeakObjectRef.h:32 > +class JSWeakObjectRef final : public JSObject { The base class needs to be JSNonFinalObject. > Source/JavaScriptCore/runtime/JSWeakObjectRef.h:34 > + using Base = JSObject; Ditto. > Source/JavaScriptCore/runtime/WeakObjectRefConstructor.cpp:76 > + JSWeakObjectRef* WeakObjectRef = JSWeakObjectRef::create(vm, WeakObjectRefStructure, exec->uncheckedArgument(0).getObject()); > + RETURN_IF_EXCEPTION(scope, encodedJSValue()); > + return JSValue::encode(WeakObjectRef); We can make this as `RELEASE_AND_RETURN(scope, JSWeakObjectRef::create(vm, WeakObjectRefStructure, exec->uncheckedArgument(0).getObject()));`. > Source/JavaScriptCore/runtime/WeakObjectRefConstructor.h:30 > +class WeakObjectRefConstructor : public InternalFunction { Annotate it with `final`. > Source/JavaScriptCore/runtime/WeakObjectRefConstructor.h:32 > + typedef InternalFunction Base; Use `using`. > Source/JavaScriptCore/runtime/WeakObjectRefConstructor.h:51 > +}; Add `static_assert(sizeof(WeakObjectRefConstructor) == sizeof(InternalFunction));`, to ensure that WeakObjectRefConstructor is OK to be allocated from IsoSubspace of InternalFunction. > Source/JavaScriptCore/runtime/WeakObjectRefPrototype.h:34 > + typedef JSNonFinalObject Base; Use `using`.
Comment on attachment 372046 [details] Patch The microqueue change looks sane to me.
Comment on attachment 372046 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372046&action=review Fixed the other things they were because I just copied the code from JSWeakSet equivalent files. I'll follow up in another bug with the fixes for those files too. >> Source/JavaScriptCore/runtime/JSWeakObjectRef.h:32 >> +class JSWeakObjectRef final : public JSObject { > > The base class needs to be JSNonFinalObject. Whoops! Nice catch.
Created attachment 372370 [details] Patch for landing
Comment on attachment 372370 [details] Patch for landing Clearing flags on attachment: 372370 Committed r246565: <https://trac.webkit.org/changeset/246565>
All reviewed patches have been landed. Closing bug.
<rdar://problem/51864685>
It's great to see WeakRefs landed in JSC. The write barrier trick to keep the object alive when deref'd is nice. I don't think any of the changes that the spec has undergone in the months since then affect the contents of this patch. Were you planning on implementing FinalizationRegistry, and shipping the pair? After a lot of discussion at TC39, including making cleanupSome "normative option" following JSC's concerns (in practice, I'd exclude it for now, as documented in the HTML integration patch), I think we can consider both the WeakRef and FinalizationRegistry specs stable; based on this, V8/Chrome have shipped the two.
(In reply to Daniel Ehrenberg from comment #29) > It's great to see WeakRefs landed in JSC. The write barrier trick to keep > the object alive when deref'd is nice. I don't think any of the changes that > the spec has undergone in the months since then affect the contents of this > patch. > > Were you planning on implementing FinalizationRegistry, and shipping the > pair? After a lot of discussion at TC39, including making cleanupSome > "normative option" following JSC's concerns (in practice, I'd exclude it for > now, as documented in the HTML integration patch), I think we can consider > both the WeakRef and FinalizationRegistry specs stable; based on this, > V8/Chrome have shipped the two. I have a patch for the old FinalizationGroup API but I haven't had a chance to update it for the new Registry version. Fortunately, that's mostly just deleting all the iterator code. Right now I'm focusing on cleaning up my for-of optimizations that I have been working on though. V8/Chrome shipping FinalizationRegistry seems reasonable to me; I would be very surprised if anything new came up with the new API.