Bug 168320 – [JSC] Drop PassRefPtr

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Drop PassRefPtr

[Yusuke Suzuki]

Created attachment 301502 [details]
Patch

[WebKit Commit Bot]

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.

[Yusuke Suzuki]

Created attachment 301506 [details]
Patch

[WebKit Commit Bot]

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.

[Yusuke Suzuki]

Created attachment 301522 [details]
Patch

[Yusuke Suzuki]

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.

[WebKit Commit Bot]

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.

[Saam Barati]

Comment on attachment 301522 [details]
Patch

r=me with it building and passing tests

[Yusuke Suzuki]

Created attachment 301609 [details]
Patch for landing

[WebKit Commit Bot]

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.

[Yusuke Suzuki]

Created attachment 301610 [details]
Patch for landing

[WebKit Commit Bot]

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.

[Yusuke Suzuki]

Committed r212365: <http://trac.webkit.org/changeset/212365>

[Filip Pizlo]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Reopening to attach new patch.

[Yusuke Suzuki]

Created attachment 301792 [details]
Patch

Patch for landing

[Yusuke Suzuki]

https://trac.webkit.org/changeset/212452 fixed it.
Thanks Fil!