Bug 107249 - [V8] We should set a class id for a NPObject wrapper
Summary: [V8] We should set a class id for a NPObject wrapper
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-18 02:39 PST by Kentaro Hara
Modified: 2013-01-20 23:20 PST (History)
3 users (show)

See Also:


Attachments
Patch (4.72 KB, patch)
2013-01-18 02:46 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (4.69 KB, patch)
2013-01-20 21:55 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 1 Kentaro Hara 2013-01-18 02:46:40 PST
Created attachment 183414 [details]
Patch
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 4 Kentaro Hara 2013-01-20 21:55:05 PST
Created attachment 183709 [details]
Patch
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 6 Adam Barth 2013-01-20 22:04:36 PST
Comment on attachment 183709 [details]
Patch

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.