Bug 97703 - [V8] Make v8NPObjectMap per Context
Summary: [V8] Make v8NPObjectMap per Context
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fady Samuel
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-26 11:01 PDT by Fady Samuel
Modified: 2012-09-28 12:38 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.21 KB, patch)
2012-09-26 11:08 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (4.36 KB, patch)
2012-09-26 11:45 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (4.28 KB, patch)
2012-09-26 11:47 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (6.89 KB, patch)
2012-09-27 13:24 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (6.76 KB, patch)
2012-09-28 08:32 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (6.75 KB, patch)
2012-09-28 11:34 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fady Samuel 2012-09-26 11:01:45 PDT
[Chromium] Cache hash of global object in V8NPObject so that it can be cleared from staticNPObjectMap after the global object has been disposed
Comment 1 Fady Samuel 2012-09-26 11:08:30 PDT
Created attachment 165835 [details]
Patch
Comment 2 Adam Barth 2012-09-26 11:19:07 PDT
Can we write a test for this issue?
Comment 3 Fady Samuel 2012-09-26 11:45:18 PDT
Created attachment 165841 [details]
Patch
Comment 4 Fady Samuel 2012-09-26 11:46:21 PDT
(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@.
Comment 5 Fady Samuel 2012-09-26 11:47:17 PDT
Created attachment 165842 [details]
Patch
Comment 6 Fady Samuel 2012-09-27 13:24:21 PDT
Created attachment 166051 [details]
Patch
Comment 7 Adam Barth 2012-09-27 14:16:21 PDT
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.
Comment 8 Adam Barth 2012-09-27 14:17:03 PDT
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.
Comment 9 Fady Samuel 2012-09-28 08:32:29 PDT
Created attachment 166258 [details]
Patch
Comment 10 Fady Samuel 2012-09-28 08:33:04 PDT
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 11 Adam Barth 2012-09-28 09:09:37 PDT
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.
Comment 12 Fady Samuel 2012-09-28 11:34:48 PDT
Created attachment 166285 [details]
Patch
Comment 13 WebKit Review Bot 2012-09-28 12:38:30 PDT
Comment on attachment 166285 [details]
Patch

Clearing flags on attachment: 166285

Committed r129933: <http://trac.webkit.org/changeset/129933>
Comment 14 WebKit Review Bot 2012-09-28 12:38:34 PDT
All reviewed patches have been landed.  Closing bug.