Bug 204556 - [JSC] InternalFunction should be non-destructible
Summary: [JSC] InternalFunction should be non-destructible
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-11-24 04:45 PST by Yusuke Suzuki
Modified: 2019-11-25 23:03 PST (History)
7 users (show)

See Also:


Attachments
Patch (4.20 KB, patch)
2019-11-24 04:46 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (6.74 KB, patch)
2019-11-24 04:50 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (7.01 KB, patch)
2019-11-24 04:59 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (8.03 KB, patch)
2019-11-24 05:05 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews214 for win-future (1.04 MB, application/zip)
2019-11-24 08:12 PST, Build Bot
no flags Details
Archive of layout-test-results from ews213 for win-future (1.02 MB, application/zip)
2019-11-24 09:48 PST, Build Bot
no flags Details
Patch (17.20 KB, patch)
2019-11-24 23:38 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (17.41 KB, patch)
2019-11-25 00:20 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (19.05 KB, patch)
2019-11-25 00:55 PST, Yusuke Suzuki
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2019-11-24 04:45:36 PST
[JSC] InternalFunction should be non-destructible
Comment 1 Yusuke Suzuki 2019-11-24 04:46:22 PST
Created attachment 384253 [details]
Patch
Comment 2 Yusuke Suzuki 2019-11-24 04:50:30 PST
Created attachment 384254 [details]
Patch
Comment 3 Yusuke Suzuki 2019-11-24 04:59:37 PST
Created attachment 384255 [details]
Patch
Comment 4 Yusuke Suzuki 2019-11-24 05:05:00 PST
Created attachment 384256 [details]
Patch
Comment 5 Yusuke Suzuki 2019-11-24 05:09:19 PST
Oops, this depends on https://bugs.webkit.org/show_bug.cgi?id=204555
Comment 6 Build Bot 2019-11-24 08:12:31 PST
Comment on attachment 384256 [details]
Patch

Attachment 384256 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13276861

Number of test failures exceeded the failure limit.
Comment 7 Build Bot 2019-11-24 08:12:33 PST
Created attachment 384257 [details]
Archive of layout-test-results from ews214 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews214  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 8 Build Bot 2019-11-24 09:48:05 PST
Comment on attachment 384256 [details]
Patch

Attachment 384256 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13276945

Number of test failures exceeded the failure limit.
Comment 9 Build Bot 2019-11-24 09:48:07 PST
Created attachment 384259 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 10 Yusuke Suzuki 2019-11-24 23:38:52 PST
Created attachment 384271 [details]
Patch
Comment 11 Yusuke Suzuki 2019-11-25 00:20:01 PST
Created attachment 384276 [details]
Patch
Comment 12 Yusuke Suzuki 2019-11-25 00:55:46 PST
Created attachment 384278 [details]
Patch
Comment 13 Mark Lam 2019-11-25 08:56:19 PST
Comment on attachment 384278 [details]
Patch

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

r=me if EWS bots are green.

> Source/JavaScriptCore/ChangeLog:11
> +        InternalFunction should be non-destructible since nobody is using destroy function.
> +        If it is defined, it should have different IsoSubspace. We annotate needsDestruction = true for
> +        these classes. And we define HeapCellType for each such a type, and pass it to IsoSubspace to
> +        invoke appropriate destructor.

I suggest rephrasing this as:

InternalFunction and most of its subclasses should be non-destructible since they can be trivially destructed and don't use a destroy function.  For the few subclasses that do need a destroy function, these should have different IsoSubspaces of their own.  For each of these subclasses, we annotate needsDestruction = true, define a specific HeapCellType for them, and pass the HeapCellType to their IsoSubspace so that their destructors can be invoked.
Comment 14 Yusuke Suzuki 2019-11-25 23:01:55 PST
Comment on attachment 384278 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:11
>> +        invoke appropriate destructor.
> 
> I suggest rephrasing this as:
> 
> InternalFunction and most of its subclasses should be non-destructible since they can be trivially destructed and don't use a destroy function.  For the few subclasses that do need a destroy function, these should have different IsoSubspaces of their own.  For each of these subclasses, we annotate needsDestruction = true, define a specific HeapCellType for them, and pass the HeapCellType to their IsoSubspace so that their destructors can be invoked.

Sounds nice!!! Fixed.
Comment 15 Yusuke Suzuki 2019-11-25 23:02:50 PST
Committed r252875: <https://trac.webkit.org/changeset/252875>
Comment 16 Radar WebKit Bug Importer 2019-11-25 23:03:20 PST
<rdar://problem/57483799>