[V8] Multiple NPObjects can be created from a single v8::Object

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nate Chapin]

Passing the same v8 wrapper (that isn't already wrapping an NPObject*) to a plugin twice will result in two different NPObjects being created and passed to the plugin.  It really should be two pointers to the same object.

[Nate Chapin]

Created attachment 94882 [details]
patch

This patch makes use of v8::Object::GetIdentityHash(), an item in the v8 api previously unused in WebCore.

Anton tells me it isn't free to use, but given that creating V8NPObjects is relatively uncommon and that this is the only sane way to use a v8::Object as a key in a map, I'm inclined to use it.

[WebKit Review Bot]

Comment on attachment 94882 [details]
patch

Attachment 94882 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8736157

New failing tests:
plugins/document-open.html
plugins/npruntime/evaluate.html
plugins/npruntime/get-int-identifier-special-values.html

[WebKit Review Bot]

Created attachment 94923 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[anton muhin]

Comment on attachment 94882 [details]
patch

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

LGTM

> Source/WebCore/bindings/v8/NPV8Object.cpp:65
> +static HashMap<int, V8NPObject*> staticV8NPObjectMap;

AFAIK, WebKit is rather paranoid about statics.  Shouldn't it be wrapped into function with DEFINE_LOCAL_STATIC macro?

> Source/WebCore/bindings/v8/NPV8Object.cpp:76
> +    staticV8NPObjectMap.remove(v8NpObject->v8Object->GetIdentityHash());

maybe assert that identity hash is in the map before removing it

> Source/WebCore/bindings/v8/NPV8Object.cpp:136
> +        return reinterpret_cast<NPObject*>(v8npObject);

looks somewhat sketchy: I'd expect (just from names) that V8NPObject is a subclass of NPObject and hence cast is not needed, but you should know better

> Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp:1037
> +    if (!browser->invokeDefault(obj->npp, func, (const NPVariant*) &func_args, 2, &func_result))

hmm, sketchy C-style cast (const NPVarian*), and is it needed at all?

And do you want to play safe and pass not 2, but ARRAY_SIZE as a size argument?  But it might be too cumbersome.

> LayoutTests/plugins/npruntime/embed-property-equality.html:12
> +    // Tests that a JavaScript object that is passed through NPAPI retains it's JavaScript identity.

nit: it's -> its

[Nate Chapin]

Comment on attachment 94882 [details]
patch

Clearing r? since it appears those chromium-ews breakages are legit.

[Nate Chapin]

New patch coming shortly.

> > Source/WebCore/bindings/v8/NPV8Object.cpp:136
> > +        return reinterpret_cast<NPObject*>(v8npObject);
> 
> looks somewhat sketchy: I'd expect (just from names) that V8NPObject is a subclass of NPObject and hence cast is not needed, but you should know better

V8NPObject is actually a struct and its first element is its NPObject.
http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/v8/NPV8Object.h#L59

[Nate Chapin]

Created attachment 95036 [details]
patch #2

* Cleaned up PluginObject.cpp, made names and variables a little more like the rest of the file.
* Added call to _NPN_RetainObject when we use a cached V8NPObject, so we don't use-after-free :)
* Added handling in freeV8NPObject for a GetIdentityHash() = 0, which happens when the v8 context is gone.

[anton muhin]

Comment on attachment 95036 [details]
patch #2

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

Still LGTM

> Source/WebCore/bindings/v8/NPV8Object.cpp:151
> +        return reinterpret_cast<NPObject*>(v8npObject);

I am not sure it's safe from point of view of the Holy Standard.  And I would definitely prefer &(v8npObject->object) which (IMHO) communicates intent ways more clear.

Feel free to ignore though.

[Darin Fisher (:fishd, Google)]

Comment on attachment 95036 [details]
patch #2

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

R=me (deferring to Anton's LGTM)

>> Source/WebCore/bindings/v8/NPV8Object.cpp:151
>> +        return reinterpret_cast<NPObject*>(v8npObject);
> 
> I am not sure it's safe from point of view of the Holy Standard.  And I would definitely prefer &(v8npObject->object) which (IMHO) communicates intent ways more clear.
> 
> Feel free to ignore though.

This is a good point.  Although it seems like the code is already using
reinterpret_cast like this in other places.  Maybe a cleanup CL?  It
would probably also be nice to create toNPObject / toV8NPObject helpers.

[Nate Chapin]

Comment on attachment 95036 [details]
patch #2

Yeah, I'll do the reinterpret_cast cleanup in a separate patch.  cq+ing.

[WebKit Commit Bot]

Comment on attachment 95036 [details]
patch #2

Rejecting attachment 95036 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build-..." exit_code: 2

Last 500 characters of output:
tests/xmlhttprequest ................................................................................................................................................................................
http/tests/xmlhttprequest/web-apps ...............
http/tests/xmlhttprequest/workers ...........
http/tests/xmlviewer .
http/tests/xmlviewer/dumpAsText ...........
747.74s total testing time

23676 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
16 test cases (<1%) had stderr output

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

[WebKit Commit Bot]

Created attachment 95246 [details]
Archive of layout-test-results from cr-jail-8

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: cr-jail-8  Port: Mac  Platform: Mac OS X 10.6.7

[Nate Chapin]

Created attachment 95631 [details]
Updated test

This patch removes the invokeWithSame() case from the layout test, since it wasn't actually relevant to this particular issue and the test was bug with JSC.

[WebKit Review Bot]

Comment on attachment 95631 [details]
Updated test

Clearing flags on attachment: 95631

Committed r88679: <http://trac.webkit.org/changeset/88679>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.