Bug 97703 – [V8] Make v8NPObjectMap per Context

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

[Fady Samuel]

[Chromium] Cache hash of global object in V8NPObject so that it can be cleared from staticNPObjectMap after the global object has been disposed

[Fady Samuel]

Created attachment 165835 [details]
Patch

[Adam Barth]

Can we write a test for this issue?

[Fady Samuel]

Created attachment 165841 [details]
Patch

[Fady Samuel]

(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@.

[Fady Samuel]

Created attachment 165842 [details]
Patch

[Fady Samuel]

Created attachment 166051 [details]
Patch

[Adam Barth]

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.

[Adam Barth]

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.

[Fady Samuel]

Created attachment 166258 [details]
Patch

[Fady Samuel]

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.

[Adam Barth]

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.

[Fady Samuel]

Created attachment 166285 [details]
Patch

[WebKit Review Bot]

Comment on attachment 166285 [details]
Patch

Clearing flags on attachment: 166285

Committed r129933: <http://trac.webkit.org/changeset/129933>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.