Bug 198710 – Add support for WeakRef

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Keith Miller]

Add support for WeakRef

[Keith Miller]

Created attachment 371736 [details]
Patch

[Michael Saboff]

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?

[Keith Miller]

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.

[Saam Barati]

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?

[Saam Barati]

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

[Keith Miller]

Created attachment 371952 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Keith Miller]

Created attachment 372038 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Keith Miller]

Created attachment 372046 [details]
Patch

[Yusuke Suzuki]

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

[Ryosuke Niwa]

Comment on attachment 372046 [details]
Patch

The microqueue change looks sane to me.

[Keith Miller]

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.

[Keith Miller]

Created attachment 372370 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 372370 [details]
Patch for landing

Clearing flags on attachment: 372370

Committed r246565: <https://trac.webkit.org/changeset/246565>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51864685>

[Daniel Ehrenberg]

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.

[Keith Miller]

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