Bug 109065 – [v8] move persistent::new and ::dispose into same class

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

[Dan Carney]

[v8] move persistent::new and ::dispose into same class

[Dan Carney]

Created attachment 186875 [details]
Patch

[Dan Carney]

There's no reason to ever pass around v8::Persistent objects, and the misuse use them makes it easy to generate memory leaks. This patch fixes that problem for ScriptWrappable and DomDataStore.

I haven't rebaselined the bindings test classed, but i'll do that before commit

[Adam Barth]

I don't really understand what problem this patch solves.  Is this just about making the code easier to understand?

[Dan Carney]

(In reply to comment #3)
> I don't really understand what problem this patch solves.  Is this just about making the code easier to understand?

i just want the Persistent::New and Persistent::Dispose in the same spot, and to ensure Persistent::Clear is called on the correct object.  Often, the wrong thing is cleared (the handle coming in on the weak callback and not the handle that created the weak callback).

[Adam Barth]

Comment on attachment 186875 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2183
> -    V8DOMWrapper::associateObjectWithWrapper(impl.release(), &${v8InterfaceName}Constructor::info, wrapper, args.GetIsolate());
> +    V8DOMWrapper::associateObjectWithWrapper(impl.release(), &${v8InterfaceName}Constructor::info, wrapper, args.GetIsolate(), true);

Is it possible that we could avoid having to pass "true" here?  How did we used to figure this out?

[Adam Barth]

Comment on attachment 186875 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3545
> -    v8::Persistent<v8::Object> wrapperHandle = V8DOMWrapper::associateObjectWithWrapper(impl, &info, wrapper, isolate);
> -    if (!hasDependentLifetime)
> -        wrapperHandle.MarkIndependent();
> +    V8DOMWrapper::associateObjectWithWrapper(impl, &info, wrapper, isolate, hasDependentLifetime);

I see.  We used to do it here.  Maybe we should use an enum instead of a boolean parameter.

[Dan Carney]

Comment on attachment 186875 [details]
Patch

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

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3545
>> +    V8DOMWrapper::associateObjectWithWrapper(impl, &info, wrapper, isolate, hasDependentLifetime);
> 
> I see.  We used to do it here.  Maybe we should use an enum instead of a boolean parameter.

I could add the WrapperConfiguration at this stage and pass that.  It would be easier to read.

[Adam Barth]

Comment on attachment 186875 [details]
Patch

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

Generally this patch looks fine.  I would prefer if the associateObjectWithWrapper didn't have a raw boolean parameter though.  Either an enum or a WrapperConfiguration seems better.

> Source/WebCore/bindings/v8/ScriptWrappable.h:47
> +        if (!m_maskedWrapper.IsEmpty())
> +            CRASH();

This part of the patch seems separable from the rest of this work.  Perhaps we should make this change in a separate patch?

> Source/WebCore/bindings/v8/ScriptWrappable.h:79
>      v8::Persistent<v8::Object> m_maskedWrapper;

Aside from the masking, it seems like we could use a ScopedPersistent here.

[Dan Carney]

Comment on attachment 186875 [details]
Patch

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

>> Source/WebCore/bindings/v8/ScriptWrappable.h:47
>> +            CRASH();
> 
> This part of the patch seems separable from the rest of this work.  Perhaps we should make this change in a separate patch?

sure. i can remove it or replace it with an assert in another patch. i thought it might help block a potential use after free exploit, but I understood the ordering wrong.

>> Source/WebCore/bindings/v8/ScriptWrappable.h:79
>>      v8::Persistent<v8::Object> m_maskedWrapper;
> 
> Aside from the masking, it seems like we could use a ScopedPersistent here.

Yeah, if I can get the masking removed at some point, I'll just replace it.

[Dan Carney]

Created attachment 187015 [details]
Patch

[Dan Carney]

Created attachment 187016 [details]
Patch

[Dan Carney]

Comment on attachment 187016 [details]
Patch

comments addressed - bindings tests rebased

[Kentaro Hara]

Comment on attachment 187016 [details]
Patch

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

> Source/WebCore/bindings/v8/WrapperTypeInfo.h:148
> +            DEPENDENT, INDEPENDENT

Per the style guide, this should be:

  Dependent, Independent

[Dan Carney]

Created attachment 187063 [details]
Patch

[Dan Carney]

Comment on attachment 187063 [details]
Patch

enum values names fixed

[Kentaro Hara]

Comment on attachment 187063 [details]
Patch

Thanks!

[WebKit Review Bot]

Comment on attachment 187063 [details]
Patch

Rejecting attachment 187063 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 187063, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:

patching file Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp
patching file Source/WebCore/bindings/v8/custom/V8MessageChannelCustom.cpp
patching file Source/WebCore/bindings/v8/custom/V8MutationObserverCustom.cpp
patching file Source/WebCore/bindings/v8/custom/V8WebKitPointCustom.cpp
patching file Source/WebCore/bindings/v8/custom/V8XMLHttpRequestCustom.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/16434205

[Dan Carney]

Comment on attachment 187063 [details]
Patch

rebased

[WebKit Review Bot]

Comment on attachment 187063 [details]
Patch

Rejecting attachment 187063 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 187063, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:

patching file Source/WebCore/bindings/v8/custom/V8IntentCustom.cpp
patching file Source/WebCore/bindings/v8/custom/V8MessageChannelCustom.cpp
patching file Source/WebCore/bindings/v8/custom/V8MutationObserverCustom.cpp
patching file Source/WebCore/bindings/v8/custom/V8WebKitPointCustom.cpp
patching file Source/WebCore/bindings/v8/custom/V8XMLHttpRequestCustom.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/16430231

[Dan Carney]

Created attachment 187130 [details]
Patch

[Dan Carney]

Comment on attachment 187130 [details]
Patch

rebased

[WebKit Review Bot]

Comment on attachment 187130 [details]
Patch

Clearing flags on attachment: 187130

Committed r142159: <http://trac.webkit.org/changeset/142159>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.