[v8] move persistent::new and ::dispose into same class
Created attachment 186875 [details] Patch
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
I don't really understand what problem this patch solves. Is this just about making the code easier to understand?
(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).
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?
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.
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.
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.
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.
Created attachment 187015 [details] Patch
Created attachment 187016 [details] Patch
Comment on attachment 187016 [details] Patch comments addressed - bindings tests rebased
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
Created attachment 187063 [details] Patch
Comment on attachment 187063 [details] Patch enum values names fixed
Comment on attachment 187063 [details] Patch Thanks!
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
Comment on attachment 187063 [details] Patch rebased
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
Created attachment 187130 [details] Patch
Comment on attachment 187130 [details] Patch rebased
Comment on attachment 187130 [details] Patch Clearing flags on attachment: 187130 Committed r142159: <http://trac.webkit.org/changeset/142159>
All reviewed patches have been landed. Closing bug.