[Chromium] Cache hash of global object in V8NPObject so that it can be cleared from staticNPObjectMap after the global object has been disposed
Created attachment 165835 [details] Patch
Can we write a test for this issue?
Created attachment 165841 [details] Patch
(In reply to comment #2) > Can we write a test for this issue? I will try. I updated the Changelog description according to a discussion with wez@.
Created attachment 165842 [details] Patch
Created attachment 166051 [details] Patch
Comment on attachment 166051 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166051&action=review > Source/WebCore/bindings/v8/NPV8Object.cpp:77 > + if (!v8NpObject->v8Object->CreationContext().IsEmpty()) { How can the creation context be empty? That doesn't make any sense. Every object must have been created in a context. > Source/WebCore/bindings/v8/NPV8Object.cpp:80 > + if (int v8ObjectHash = v8NpObject->v8Object->GetIdentityHash()) { Can this > Source/WebCore/bindings/v8/NPV8Object.cpp:93 > + } else > + ASSERT_NOT_REACHED(); Rather than structuring this as an if-then-else NOT REACHED, we should just ASSERT the condition in the if-block and continue. > Source/WebCore/bindings/v8/NPV8Object.cpp:96 > + ASSERT(!v8::Context::InContext()); > + v8NPObjectMap->clear(); Can this case occur now? I would expect that the perContextData would be gone before the hashes start turning into zeros.
Would you be willing to write a chromium-side test for this issue? Also, I would run this patch through the Chromium try servers to see if that uncovers any issues with this approach.
Created attachment 166258 [details] Patch
Comment on attachment 166051 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166051&action=review >> Source/WebCore/bindings/v8/NPV8Object.cpp:77 >> + if (!v8NpObject->v8Object->CreationContext().IsEmpty()) { > > How can the creation context be empty? That doesn't make any sense. Every object must have been created in a context. Changing it to an ASSERT instead. >> Source/WebCore/bindings/v8/NPV8Object.cpp:80 >> + if (int v8ObjectHash = v8NpObject->v8Object->GetIdentityHash()) { > > Can this I've added an ASSERT(v8ObjectHash) instead. >> Source/WebCore/bindings/v8/NPV8Object.cpp:93 >> + ASSERT_NOT_REACHED(); > > Rather than structuring this as an if-then-else NOT REACHED, we should just ASSERT the condition in the if-block and continue. Done. >> Source/WebCore/bindings/v8/NPV8Object.cpp:96 >> + v8NPObjectMap->clear(); > > Can this case occur now? I would expect that the perContextData would be gone before the hashes start turning into zeros. Yup, added an ASSERT that the hash is non-zero.
Comment on attachment 166258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166258&action=review This looks great. I don't have a complete enough understanding of how plugins work to know if this is going to work in all cases, but I think we should give it a try. > Source/WebCore/bindings/v8/NPV8Object.cpp:79 > + V8NPObjectMap* v8NPObjectMap = perContextData->getV8NPObjectMap(); getV8NPObjectMap -> v8NPObjectMap We tend to avoid the "get" prefix in names because it doesn't really mean anything.
Created attachment 166285 [details] Patch
Comment on attachment 166285 [details] Patch Clearing flags on attachment: 166285 Committed r129933: <http://trac.webkit.org/changeset/129933>
All reviewed patches have been landed. Closing bug.