RESOLVED FIXED 61482
[V8] Multiple NPObjects can be created from a single v8::Object
https://bugs.webkit.org/show_bug.cgi?id=61482
Summary [V8] Multiple NPObjects can be created from a single v8::Object
Nate Chapin
Reported 2011-05-25 16:45:22 PDT
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.
Attachments
patch (7.75 KB, patch)
2011-05-25 17:02 PDT, Nate Chapin
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-01 (1.23 MB, application/zip)
2011-05-25 23:19 PDT, WebKit Review Bot
no flags
patch #2 (8.20 KB, patch)
2011-05-26 14:02 PDT, Nate Chapin
fishd: review+
commit-queue: commit-queue-
Archive of layout-test-results from cr-jail-8 (207.86 KB, application/zip)
2011-05-27 19:25 PDT, WebKit Commit Bot
no flags
Updated test (6.72 KB, patch)
2011-06-01 11:19 PDT, Nate Chapin
no flags
Nate Chapin
Comment 1 2011-05-25 17:02:48 PDT
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 2 2011-05-25 23:19:00 PDT
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
Comment 3 2011-05-25 23:19:04 PDT
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 4 2011-05-26 04:21:40 PDT
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 5 2011-05-26 09:50:54 PDT
Comment on attachment 94882 [details] patch Clearing r? since it appears those chromium-ews breakages are legit.
Nate Chapin
Comment 6 2011-05-26 13:50:30 PDT
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
Comment 7 2011-05-26 14:02:26 PDT
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 8 2011-05-27 04:57:20 PDT
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 9 2011-05-27 15:29:36 PDT
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 10 2011-05-27 15:34:05 PDT
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 11 2011-05-27 19:25:21 PDT
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
Comment 12 2011-05-27 19:25:25 PDT
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
Comment 13 2011-06-01 11:19:31 PDT
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 14 2011-06-13 13:19:20 PDT
Comment on attachment 95631 [details] Updated test Clearing flags on attachment: 95631 Committed r88679: <http://trac.webkit.org/changeset/88679>
WebKit Review Bot
Comment 15 2011-06-13 13:19:27 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.