Bug 184721 - The InternalFunction hierarchy should be in IsoSubspaces
Summary: The InternalFunction hierarchy should be in IsoSubspaces
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-04-17 17:31 PDT by Filip Pizlo
Modified: 2018-04-19 12:34 PDT (History)
3 users (show)

See Also:


Attachments
possibly the patch (45.21 KB, patch)
2018-04-17 17:34 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
maybe the patch (47.05 KB, patch)
2018-04-17 17:42 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
maybe the patch (47.05 KB, patch)
2018-04-17 18:46 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
maybe the patch (47.06 KB, patch)
2018-04-17 19:02 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (47.42 KB, patch)
2018-04-17 19:22 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (47.42 KB, patch)
2018-04-17 19:24 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (47.42 KB, patch)
2018-04-17 19:52 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (47.52 KB, patch)
2018-04-17 20:01 PDT, Filip Pizlo
saam: review+
Details | Formatted Diff | Diff
patch for landing (47.40 KB, patch)
2018-04-19 11:17 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2018-04-17 17:31:06 PDT
With the caveat that subclasses of InternalFunction that don't add new fields may as well share the same IsoSubspace as InternalFunction, since I can't see a security downside of doing so. They will dynamically figure out what to do based on state in JSCell, JSObject, JSDestructibleObject, and InternalFunction.
Comment 1 Filip Pizlo 2018-04-17 17:34:02 PDT
Created attachment 338169 [details]
possibly the patch
Comment 2 EWS Watchlist 2018-04-17 17:37:12 PDT
Attachment 338169 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/Plugins/Netscape/JSNPMethod.cpp:66:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/bindings/js/WebCoreJSClientData.cpp:38:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Source/WebKit/WebProcess/Plugins/Netscape/JSNPObject.cpp:534:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 3 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Filip Pizlo 2018-04-17 17:42:52 PDT
Created attachment 338171 [details]
maybe the patch
Comment 4 EWS Watchlist 2018-04-17 17:44:20 PDT
Attachment 338171 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/Plugins/Netscape/JSNPMethod.cpp:66:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/bindings/js/WebCoreJSClientData.cpp:38:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Source/WebKit/WebProcess/Plugins/Netscape/JSNPObject.cpp:534:  More than one command on the same line  [whitespace/newline] [4]
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCCallbackFunction.h"
ERROR: Source/JavaScriptCore/API/glib/JSCCallbackFunction.cpp:212:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 4 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Filip Pizlo 2018-04-17 18:46:15 PDT
Created attachment 338180 [details]
maybe the patch
Comment 6 EWS Watchlist 2018-04-17 18:48:07 PDT
Attachment 338180 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/Plugins/Netscape/JSNPMethod.cpp:66:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/bindings/js/WebCoreJSClientData.cpp:38:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Source/WebKit/WebProcess/Plugins/Netscape/JSNPObject.cpp:534:  More than one command on the same line  [whitespace/newline] [4]
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCCallbackFunction.h"
ERROR: Source/JavaScriptCore/API/glib/JSCCallbackFunction.cpp:212:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 4 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Filip Pizlo 2018-04-17 19:02:34 PDT
Created attachment 338181 [details]
maybe the patch
Comment 8 EWS Watchlist 2018-04-17 19:05:39 PDT
Attachment 338181 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/Plugins/Netscape/JSNPMethod.cpp:66:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/bindings/js/WebCoreJSClientData.cpp:38:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Source/WebKit/WebProcess/Plugins/Netscape/JSNPObject.cpp:534:  More than one command on the same line  [whitespace/newline] [4]
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCCallbackFunction.h"
ERROR: Source/JavaScriptCore/API/glib/JSCCallbackFunction.cpp:212:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 4 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Filip Pizlo 2018-04-17 19:22:03 PDT
Created attachment 338183 [details]
the patch
Comment 10 Filip Pizlo 2018-04-17 19:24:47 PDT
Created attachment 338184 [details]
the patch
Comment 11 EWS Watchlist 2018-04-17 19:26:10 PDT
Attachment 338184 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/Plugins/Netscape/JSNPMethod.cpp:66:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/bindings/js/WebCoreJSClientData.cpp:38:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Source/WebKit/WebProcess/Plugins/Netscape/JSNPObject.cpp:534:  More than one command on the same line  [whitespace/newline] [4]
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCCallbackFunction.h"
ERROR: Source/JavaScriptCore/API/glib/JSCCallbackFunction.cpp:212:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 4 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Filip Pizlo 2018-04-17 19:52:02 PDT
Created attachment 338188 [details]
the patch
Comment 13 EWS Watchlist 2018-04-17 19:54:05 PDT
Attachment 338188 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/Plugins/Netscape/JSNPMethod.cpp:66:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/bindings/js/WebCoreJSClientData.cpp:38:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Source/WebKit/WebProcess/Plugins/Netscape/JSNPObject.cpp:534:  More than one command on the same line  [whitespace/newline] [4]
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCCallbackFunction.h"
ERROR: Source/JavaScriptCore/API/glib/JSCCallbackFunction.cpp:212:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 4 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Filip Pizlo 2018-04-17 20:01:02 PDT
Created attachment 338189 [details]
the patch
Comment 15 EWS Watchlist 2018-04-17 20:04:50 PDT
Attachment 338189 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/Plugins/Netscape/JSNPMethod.cpp:66:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/bindings/js/WebCoreJSClientData.cpp:38:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Source/WebKit/WebProcess/Plugins/Netscape/JSNPObject.cpp:534:  More than one command on the same line  [whitespace/newline] [4]
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCCallbackFunction.h"
ERROR: Source/JavaScriptCore/API/glib/JSCCallbackFunction.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/API/glib/JSCCallbackFunction.cpp:213:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 5 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Saam Barati 2018-04-18 15:10:17 PDT
Comment on attachment 338189 [details]
the patch

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

> Source/JavaScriptCore/heap/IsoSubspacePerVM.cpp:65
> +    if (result) {

style nit: I feel result.isNewEntry is easier to read than this (I just had to look up that AddResult has operator bool that returns newEntry).
Comment 17 Filip Pizlo 2018-04-19 11:15:30 PDT
(In reply to Saam Barati from comment #16)
> Comment on attachment 338189 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338189&action=review
> 
> > Source/JavaScriptCore/heap/IsoSubspacePerVM.cpp:65
> > +    if (result) {
> 
> style nit: I feel result.isNewEntry is easier to read than this (I just had
> to look up that AddResult has operator bool that returns newEntry).

Makes sense, will change.
Comment 18 Filip Pizlo 2018-04-19 11:17:39 PDT
Created attachment 338345 [details]
patch for landing
Comment 19 EWS Watchlist 2018-04-19 11:20:02 PDT
Attachment 338345 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/Plugins/Netscape/JSNPMethod.cpp:66:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/bindings/js/WebCoreJSClientData.cpp:38:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Source/WebKit/WebProcess/Plugins/Netscape/JSNPObject.cpp:534:  More than one command on the same line  [whitespace/newline] [4]
WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCCallbackFunction.h"
ERROR: Source/JavaScriptCore/API/glib/JSCCallbackFunction.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/API/glib/JSCCallbackFunction.cpp:213:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 5 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 Filip Pizlo 2018-04-19 12:33:21 PDT
Landed in https://trac.webkit.org/changeset/230813/webkit
Comment 21 Radar WebKit Bug Importer 2018-04-19 12:34:27 PDT
<rdar://problem/39572914>