In bug 101054, we want to add an ASSERT() that guarantees that we don't pass 0 to setDOMWrapper(). On the other hand, V8NPObject.cpp is trying to pass 0 to setDOMWrapper() just before disposing a wrapper, which hits the ASSERT(). We should remove it.
Created attachment 172284 [details] Patch
Comment on attachment 172284 [details] Patch What if other folks have a reference to the v8 object? After this change, won't they have a use-after-free if they try to access the wrapped object? In the DOM case, we only remove objects from the wrapper map when we get a weak handle callback. In the case of NP objects, we can remove them when asked to do so explicitly by the plugin (you might want to check that I'm right about that---it's from memory).
(In reply to comment #2) > (From update of attachment 172284 [details]) > What if other folks have a reference to the v8 object? After this change, won't they have a use-after-free if they try to access the wrapped object? Just after this setDOMWrapper(wrapper, 0), the wrapper handle is immediately disposed and cleared. This means that other folks should not have a reference to the V8 object, doesn't it?
> Just after this setDOMWrapper(wrapper, 0), the wrapper handle is immediately disposed and cleared. This means that other folks should not have a reference to the V8 object, doesn't it? No, that just means that we don't have a reference anymore. There could be other handles to the object, either local or persistent or internal to the JS VM.
(In reply to comment #4) > > Just after this setDOMWrapper(wrapper, 0), the wrapper handle is immediately disposed and cleared. This means that other folks should not have a reference to the V8 object, doesn't it? > > No, that just means that we don't have a reference anymore. There could be other handles to the object, either local or persistent or internal to the JS VM. Yeah, makes sense... but that sounds like another problem. In bindings (even in V8NPObject.cpp), we do not do a null check for the result of toNative(handle). If there were other folks holding a handle to the wrapper, the folks might retrieve null pointers from internal fields and use them.
I think only NP objects need to null-check toNative.
(In reply to comment #6) > I think only NP objects need to null-check toNative. Thanks, now I understood things. As far as I scanned the code, the only place where toNative(npObjectWrapper) is called is weakNPObjectCallback() in V8NPObject.cpp. Just after toNative(npObjectWrapper), we have an ASSERT() that checks if the retrieved npObject is not NULL. So, the world is working safely.
Reopening to attach new patch.
Created attachment 172363 [details] Patch
Comment on attachment 172363 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172363&action=review > Source/WebCore/bindings/v8/V8DOMWrapper.h:77 > + static void resetDOMWrapper(v8::Handle<v8::Object> object, WrapperTypeInfo* type) resetDOMWrapper -> clearDOMWrapper?
Created attachment 172519 [details] patch for landing
Comment on attachment 172519 [details] patch for landing Rejecting attachment 172519 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/14690390
Created attachment 172542 [details] patch for landing
Comment on attachment 172542 [details] patch for landing Clearing flags on attachment: 172542 Committed r133589: <http://trac.webkit.org/changeset/133589>
All reviewed patches have been landed. Closing bug.