|Summary:||[V8] We should set a class id for a NPObject wrapper|
|Product:||WebKit||Reporter:||Kentaro Hara <haraken>|
|Severity:||Normal||CC:||abarth, japhet, webkit.review.bot|
|Version:||528+ (Nightly build)|
Description Kentaro Hara 2013-01-18 02:39:39 PST
This is one of steps to avoid hitting an ASSERT() that will be added in https://bugs.webkit.org/show_bug.cgi?id=107137 . We should set a class id for all wrapper objects.
Comment 2 Adam Barth 2013-01-18 11:09:02 PST
Comment on attachment 183414 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183414&action=review What effect does setting the class ID have? I think it causes us to assume the object has native info, which is true of these objects. I think this change is probably ok, but I'll need to study it more. > Source/WebCore/bindings/v8/V8NPObject.cpp:434 > + // FIXME: Move staticNPObjectMap() to DOMDataStore. There's something weird about weakNPObjectCallback, which is why I didn't do this before. Our code might be good enough for us to do it now though. > Source/WebCore/bindings/v8/V8NPObject.cpp:446 > + v8::Persistent<v8::Object> wrapperHandle = v8::Persistent<v8::Object>::New(value); > + V8DOMWrapper::setWrapperClass(object, wrapperHandle); > + staticNPObjectMap().set(object, wrapperHandle); Is it ok that we've re-ordered this operations relative to _NPN_RetainObject and _NPN_RegisterObject ?
Comment 3 Adam Barth 2013-01-20 01:00:28 PST
Comment on attachment 183414 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183414&action=review > Source/WebCore/bindings/v8/V8NPObject.cpp:445 > + V8DOMWrapper::setWrapperClass(object, wrapperHandle); I've checked, and this should be fine. The main thin we use the wrapper class for is make sure that the object has native info set. The main thing I'm worried about in this patch is the re-ordering of these calls and the _NPN_RetainObject and _NPN_RegisterObject calls. Is that necessary?
Comment 5 Kentaro Hara 2013-01-20 21:55:40 PST
(In reply to comment #3) > The main thing I'm worried about in this patch is the re-ordering of these calls and the _NPN_RetainObject and _NPN_RegisterObject calls. Is that necessary? Fixed the re-order issue. Thanks!
Comment 7 WebKit Review Bot 2013-01-20 22:51:01 PST
Comment on attachment 183709 [details] Patch Rejecting attachment 183709 [details] from commit-queue. New failing tests: inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html Full output: http://queues.webkit.org/results/16010311
Comment 8 WebKit Review Bot 2013-01-20 23:20:15 PST
Comment on attachment 183709 [details] Patch Clearing flags on attachment: 183709 Committed r140302: <http://trac.webkit.org/changeset/140302>
Comment 9 WebKit Review Bot 2013-01-20 23:20:19 PST
All reviewed patches have been landed. Closing bug.