Bug 107249 – [V8] We should set a class id for a NPObject wrapper

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Kentaro Hara]

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.

[Kentaro Hara]

Created attachment 183414 [details]
Patch

[Adam Barth]

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 ?

[Adam Barth]

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?

[Kentaro Hara]

Created attachment 183709 [details]
Patch

[Kentaro Hara]

(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!

[Adam Barth]

Comment on attachment 183709 [details]
Patch

Thanks.

[WebKit Review Bot]

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

[WebKit Review Bot]

Comment on attachment 183709 [details]
Patch

Clearing flags on attachment: 183709

Committed r140302: <http://trac.webkit.org/changeset/140302>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.