Bug 205107 – Put all generated JSCells in WebCore into IsoSubspace

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

[Yusuke Suzuki]

Do it via CodeGeneratorJS.pm

[Yusuke Suzuki]

Created attachment 392377 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/60060068>

[Yusuke Suzuki]

Created attachment 392497 [details]
Patch

[Yusuke Suzuki]

Created attachment 392504 [details]
Patch

[Yusuke Suzuki]

Created attachment 392507 [details]
Patch

[Yusuke Suzuki]

Created attachment 392521 [details]
Patch

[Saam Barati]

Comment on attachment 392521 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4679
> +        push(@implContent, "    else\n");
> +        push(@implContent, "        spaces.m_subspaceFor${interfaceName} = makeUnique<IsoSubspace> ISO_SUBSPACE_INIT(vm.heap, vm.cellHeapCellType.get(), ${className});\n");

can we also assert here that the object is not destructible. Maybe even static assert?

[Saam Barati]

r=me

[Yusuke Suzuki]

Committed r257905: <https://trac.webkit.org/changeset/257905>

[Jason Lawrence]

Reverted r257905 for reason:

This commit caused crashes on Mac wk2 Debug.

Committed r257922: <https://trac.webkit.org/changeset/257922>

[Yusuke Suzuki]

Committed r257950: <https://trac.webkit.org/changeset/257950>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 208704

[Yusuke Suzuki]

Comment on attachment 392521 [details]
Patch

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

> Source/WebCore/bindings/js/WebCoreJSClientData.h:72
> +    Vector<JSC::IsoSubspace*> outputConstraintSpaces() { return m_outputConstraintSpaces; }

This is the bug. We should return Vector<JSC::IsoSubspace*>&.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #13)
> Comment on attachment 392521 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=392521&action=review
> 
> > Source/WebCore/bindings/js/WebCoreJSClientData.h:72
> > +    Vector<JSC::IsoSubspace*> outputConstraintSpaces() { return m_outputConstraintSpaces; }
> 
> This is the bug. We should return Vector<JSC::IsoSubspace*>&.

I've ensured this fixes the debug assertion problem.

[Yusuke Suzuki]

Committed r257975: <https://trac.webkit.org/changeset/257975>

[Fujii Hironori]

Do you want to put WebCoreTestSupport objects into IsoSubspace?
It seems a layer violation that WebCore depends on WebCoreTestSupport.

[Yusuke Suzuki]

(In reply to Fujii Hironori from comment #16)
> Do you want to put WebCoreTestSupport objects into IsoSubspace?
> It seems a layer violation that WebCore depends on WebCoreTestSupport.

In Apple ports, all IDL files are preprocessed first, and at this point, we are generating IsoSubspaces.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #17)
> (In reply to Fujii Hironori from comment #16)
> > Do you want to put WebCoreTestSupport objects into IsoSubspace?
> > It seems a layer violation that WebCore depends on WebCoreTestSupport.
> 
> In Apple ports, all IDL files are preprocessed first, and at this point, we
> are generating IsoSubspaces.

Currently, in Apple ports, preprocess-idls.pl phase does not have layering between support ones and WebCore ones. All IDLs are preprocessed at once. So IsoSubspaces are generated at this point. That’s why IsoSubspaces for testing things are also generated here for now.

[Fujii Hironori]

It makes WebCore can't build without WebCoreTestSupport's IDL files.
There is no actual problem. It's just a layer matter.