[JSC] Drop PassRefPtr
Created attachment 301502 [details] Patch
Attachment 301502 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecode/ProgramCodeBlock.h:72: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1915: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/FunctionCodeBlock.h:72: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/EvalCodeBlock.h:74: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/ModuleProgramCodeBlock.h:72: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/runtime/NativeExecutable.h:42: The parameter name "function" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/Structure.h:594: 'toStructureShape' is incorrectly named. It should be named 'protector' or 'protectedJSValue'. [readability/naming/protected] [4] Total errors found: 7 in 68 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 301506 [details] Patch
Attachment 301506 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecode/ProgramCodeBlock.h:72: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1915: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/FunctionCodeBlock.h:72: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/EvalCodeBlock.h:74: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/ModuleProgramCodeBlock.h:72: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/runtime/NativeExecutable.h:42: The parameter name "function" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/Structure.h:594: 'toStructureShape' is incorrectly named. It should be named 'protector' or 'protectedJSValue'. [readability/naming/protected] [4] Total errors found: 7 in 69 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 301522 [details] Patch
Comment on attachment 301522 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301522&action=review > Source/JavaScriptCore/ChangeLog:7 > + This is the subset of the series of the patches. We do not touch inspector/, bytecode/, and ArrayBuffer related things for now.
Attachment 301522 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecode/ProgramCodeBlock.h:72: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1915: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/FunctionCodeBlock.h:72: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/EvalCodeBlock.h:74: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/ModuleProgramCodeBlock.h:72: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/runtime/NativeExecutable.h:42: The parameter name "function" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/Structure.h:594: 'toStructureShape' is incorrectly named. It should be named 'protector' or 'protectedJSValue'. [readability/naming/protected] [4] Total errors found: 7 in 67 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 301522 [details] Patch r=me with it building and passing tests
Created attachment 301609 [details] Patch for landing
Attachment 301609 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecode/ProgramCodeBlock.h:72: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1915: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/FunctionCodeBlock.h:72: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/EvalCodeBlock.h:74: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/ModuleProgramCodeBlock.h:72: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/runtime/NativeExecutable.h:42: The parameter name "function" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/Structure.h:594: 'toStructureShape' is incorrectly named. It should be named 'protector' or 'protectedJSValue'. [readability/naming/protected] [4] Total errors found: 7 in 67 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 301610 [details] Patch for landing
Attachment 301610 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecode/ProgramCodeBlock.h:72: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1915: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/FunctionCodeBlock.h:72: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/EvalCodeBlock.h:74: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/ModuleProgramCodeBlock.h:72: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/runtime/NativeExecutable.h:42: The parameter name "function" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/Structure.h:594: 'toStructureShape' is incorrectly named. It should be named 'protector' or 'protectedJSValue'. [readability/naming/protected] [4] Total errors found: 7 in 67 files If any of these errors are false positives, please file a bug against check-webkit-style.
Committed r212365: <http://trac.webkit.org/changeset/212365>
Comment on attachment 301522 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301522&action=review > Source/JavaScriptCore/API/tests/JSONParseTest.cpp:42 > - RefPtr<VM> vm = VM::create(); > + { > + Ref<VM> vm = VM::create(); This change is wrong. It's not correct to use Ref<> for VM. I think that Ref<> in general is very dangerous for JSC code, because in JSC we rely on pointers being allowed to be null and we rely on the assignability of pointers. > Source/JavaScriptCore/API/tests/JSONParseTest.cpp:-61 > - vm = nullptr; This is incorrect. We were deliberately nulling the VM, so that our reference to it would be gone before JSLockHolder destructs. This manifests as RELEASE_ASSERTs because the VM gets destructed without the lock held.
Comment on attachment 301522 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301522&action=review Thanks! >> Source/JavaScriptCore/API/tests/JSONParseTest.cpp:42 >> + Ref<VM> vm = VM::create(); > > This change is wrong. It's not correct to use Ref<> for VM. > > I think that Ref<> in general is very dangerous for JSC code, because in JSC we rely on pointers being allowed to be null and we rely on the assignability of pointers. OK, for VM, we should use RefPtr<>. I think that we already made factory functions `::create()` return `Ref<>`. So using Ref<> after that creation just propagates this non-null ability. This patch just propagates the Ref<> from the factory function to the caller side. And if the given factory function can fail, we should make this function return `RefPtr<>`. And in that case, we named these functions `tryCreate` (e.g. ArrayBuffer::tryCreate()). This returns RefPtr<>. And of course, receiving it as Ref<> causes compile error! And I think that using Ref<> if we can makes it explicit whether the given code relies on the assignability of pointers. So actually, in this change, we still keep RefPtr<> if it requires assignability. And if we use Ref<> for the code that uses assignability, it just becomes compile error. >> Source/JavaScriptCore/API/tests/JSONParseTest.cpp:-61 >> - vm = nullptr; > > This is incorrect. We were deliberately nulling the VM, so that our reference to it would be gone before JSLockHolder destructs. > > This manifests as RELEASE_ASSERTs because the VM gets destructed without the lock held. I see. I'll apply the patch to revert this VM part.
Reopening to attach new patch.
Created attachment 301792 [details] Patch Patch for landing
https://trac.webkit.org/changeset/212452 fixed it. Thanks Fil!