Comments on bug 88232 suggested that, since unwrapping effectively reinterpret_casts between the wrapped type and the type being unwrapped, to add assertions that the reinterpret_cast is safe (ie results in the same exact pointer.) We can probably do better, by always casting to the root of the type hierarchy when wrapping, and static_cast<WrapperType*>(reinterpret_cast<RootType*>(…)) when unwrapping.
Created attachment 149697 [details] Patch
Type safety could be further improved by linking the wrapper runtime type descriptors (ie V8Foo::info) and the static types in C++, perhaps by templatizing WrapperTypeInfo, but for now this should be enough to avoid changing the inheritance of SVGElementInstance in patch 146017.
Comment on attachment 149697 [details] Patch Looks OK to me
Comment on attachment 149697 [details] Patch I have vague concerns that there people will forget to static_cast before wrapping. However, I'm not immediately seeing a defense against that other than careful review, and it seems an improvement over the current quirks. LGTM
Couple of observations: 0) you potentially break a very important invariant: key in DOMMap now can be different from the pointer stored in the wrapper; that may break things like GC controller (see below); 1) you need similar changes to V8GCController---it operates on void*; 2) to make it easier to ensure presence of static cast, setDOMWrapper may become parameterized by bindings class; 3) there is unpleasant story with HTMLAudioElement which is both active object and HTMLElement, currently it's wrapped as active DOM object, I suspect your approach wont't work for it, may need manual coding.
(In reply to comment #5) > Couple of observations: > > 0) you potentially break a very important invariant: key in DOMMap now can be different from the pointer stored in the wrapper; that may break things like GC controller (see below); When does that happen? > 1) you need similar changes to V8GCController---it operates on void*; The pointers are still stored at void*, they are just cast to a base type first. > 2) to make it easier to ensure presence of static cast, setDOMWrapper may become parameterized by bindings class; I agree this would be nice. It would be nicer too if there was something that ensured the WrapperTypeInfo and the pointer being wrapped were related. From memory the thing that makes that tricky is WorkerContext wrapping which isn't static. But maybe it could be templatized too. > 3) there is unpleasant story with HTMLAudioElement which is both active object and HTMLElement, currently it's wrapped as active DOM object, I suspect your approach wont't work for it, may need manual coding. Isn't the solution just to use the WrapperTypeInfo's toActiveDOMObject? HTMLAudioElement looks the same as HTMLImageElement, etc.
(In reply to comment #6) > (In reply to comment #5) > > Couple of observations: > > > > 0) you potentially break a very important invariant: key in DOMMap now can be different from the pointer stored in the wrapper; that may break things like GC controller (see below); > > When does that happen? Consider typical ::wrapSlow after your change (just taken from the diff): V8DOMWrapper::setDOMWrapper(args.Holder(), &info, static_cast<V8WebKitMutationObserver::WrappedType*>(observer.get())); V8DOMWrapper::setJSWrapperForDOMObject(observer.release(), v8::Persistent<v8::Object>::New(args.Holder())); Now the very source of the problem is static_cast<V8WebKitMutationObserver::WrappedType*>(observer.get()) maybe different from observer.release() > > 1) you need similar changes to V8GCController---it operates on void*; > > The pointers are still stored at void*, they are just cast to a base type first. Nope, see above. And to confuse things sometimes they are retrieve from wrappers, but sometimes from map keys. > > 2) to make it easier to ensure presence of static cast, setDOMWrapper may become parameterized by bindings class; > > I agree this would be nice. It would be nicer too if there was something that ensured the WrapperTypeInfo and the pointer being wrapped were related. From memory the thing that makes that tricky is WorkerContext wrapping which isn't static. But maybe it could be templatized too. > > > 3) there is unpleasant story with HTMLAudioElement which is both active object and HTMLElement, currently it's wrapped as active DOM object, I suspect your approach wont't work for it, may need manual coding. > > Isn't the solution just to use the WrapperTypeInfo's toActiveDOMObject? HTMLAudioElement looks the same as HTMLImageElement, etc. Something like that, but toActiveDOMObject uses wrapper if memory serves.
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > Couple of observations: > > > > > > 0) you potentially break a very important invariant: key in DOMMap now can be different from the pointer stored in the wrapper; that may break things like GC controller (see below); > > > > When does that happen? > > Consider typical ::wrapSlow after your change (just taken from the diff): > > V8DOMWrapper::setDOMWrapper(args.Holder(), &info, static_cast<V8WebKitMutationObserver::WrappedType*>(observer.get())); > V8DOMWrapper::setJSWrapperForDOMObject(observer.release(), v8::Persistent<v8::Object>::New(args.Holder())); > > Now the very source of the problem is static_cast<V8WebKitMutationObserver::WrappedType*>(observer.get()) maybe different from observer.release() Good point! > > > 1) you need similar changes to V8GCController---it operates on void*; > > > > The pointers are still stored at void*, they are just cast to a base type first. > > Nope, see above. And to confuse things sometimes they are retrieve from wrappers, but sometimes from map keys. > > > > 2) to make it easier to ensure presence of static cast, setDOMWrapper may become parameterized by bindings class; > > > > I agree this would be nice. It would be nicer too if there was something that ensured the WrapperTypeInfo and the pointer being wrapped were related. From memory the thing that makes that tricky is WorkerContext wrapping which isn't static. But maybe it could be templatized too. For now I’m going to try adding an assertion into wrapSlow to unblock bug 88232. I’m going to leave this bug open because it sounds like it is worthwhile cleaning this up. FWIW the JSC bindings generate a method called impl on their wrappers to return the native type. The reintepret_cast is done at the base type and every derived wrapper just does a static cast of Base::impl() which is nice and simple. As Anton points out, V8 seems to sling pointers more, so we probably need a more methodical refactoring to attack this.
Comment on attachment 149697 [details] Patch This should not be landed as-is for reasons pointed out above.
Closing some V8-related work items.