v8 regression causes crash of Object accessed post-destruction

[GoogleCodeExporter]

From a mail just sent to the v8 list:

-----------------------------
i have just found a regression (from "sometime since last summer") in the 
handling of Object destruction. Consider this JS code:

    assert(m.destroy(), 'm.destroy()');
    assertThrows( function() {m.doFoo();} );

(m is a bound Native object)

m.destroy() simply triggers the WeakPtr destructor callback and Dispose()s the 
handle. What USED to happen after that was that any method calls could still be 
made on m (like the call to m.doFoo() above) but my framework would see that 
the Native binding in that Object (i.e. an Internal Field) was missing and 
would trigger a JS-side exception. What now happens is that the call to 
m.doFoo() triggers a v8 assertion as a side-effect of my code trying to figure 
out of m still points to a valid native. So m.doFoo() now results in the 
following assertion:

ConvertDemo.hpp:246:~BoundSubNative(): @0x1a47880 is destructing
ConvertDemo.hpp:53:~BoundNative(): @0x1a47880 is destructing.
Assertion OK: m.destroy()

#
# Fatal error in ../src/objects-inl.h, line 2386
# CHECK(object->IsJSObject()) failed
#

==== C stack trace ===============================

 1: V8_Fatal
 2: v8::internal::JSObject::cast(v8::internal::Object*)
 3: ??
 4: v8::Object::GetPointerFromInternalField(int)
 5: ??
 6: ??
 7: ??
 8: ??
 9: cvv8::Detail::MethodToInCaVoid<BoundNative, void (), &(BoundNative::doFoo()), true>::Call(v8::Arguments const&)
10: ??
11: ??
12: ??
13: ??

i don't know if this change was intentional but it hoses an inordinate amount 
of my code which assert()s that my framework can correctly avoid stepping on a 
deallocated native pointer (after the WeakPtr destruction callback is called on 
it). It also makes it painfully simple to crash the native app from script 
code, the avoidance of which is the whole purpose of my framework catching 
post-destruction access to deallocated natives.

(Yes, i know that GetPointerFromInternalField() is deprecated but 
GetAlignedPointerFromInternalField() asserts with not-aligned errors on x64!)

Is this a bug or a feature which i need to go rewire my code to deal with?
--------------------------------

This might be limited to x64 (as yet unknown, but i've seen problems like this 
before on x64 which don't show up on i32).

There is no way for us to fix this at the client level - either it must be 
fixed in v8 or cvv8 client code must never access an Object after it is 
destroyed via its WeakPtr callback (e.g. via ClassCreator::DestroyObject()).

Original issue reported on code.google.com by sgbeal@googlemail.com on 22 Dec 2012 at 1:38

[GoogleCodeExporter]

Original comment by sgbeal@googlemail.com on 22 Dec 2012 at 1:41

• Changed state: Accepted

[GoogleCodeExporter]

https://groups.google.com/forum/?fromgroups=#!topic/v8-dev/-O0FbmBqjOs

it seems that the cvv8 API will have to be majorly refactored to use 
SetHiddenField() for storing the native and (optional) type-safety check 
pointer. On Linux x64 and ia32 we're getting "pointer not aligned" assertions 
on system-allocated pointers when using the new Get/SetAlignedInternal...() 
(but not on Windows). This also breaks v8-juice, but i'm not going to go back 
and fix all of that code - it will have to either die off or be taken over by 
someone else.

<gripe>Sometimes i really hate the v8 devs. This type of thing is the reason i 
stopped coding with Qt - continually having to go back and fix/rework my apps 
when the devs change old behaviours.</gripe>

Original comment by sgbeal@googlemail.com on 23 Dec 2012 at 12:48

[GoogleCodeExporter]

The post-destruction-deref crash is fixed (incidentally) in r2259. The switch 
from Get/SetPointerInInternalField() to Get/SetInternalField(n, External) (to 
work around broken alignment requirements) incidentally worked around that 
problem.

Original comment by sgbeal@googlemail.com on 23 Dec 2012 at 2:32

• Changed state: Fixed