[webkit-reviews] review denied: [Bug 82201] [V8][Performance] Optimize createTextNode(), createElement(), cloneNode() etc : [Attachment 133802] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Kentaro Hara <haraken at chromium.org>'s
request for review:
Bug 82201: [V8][Performance] Optimize createTextNode(), createElement(),
cloneNode() etc
https://bugs.webkit.org/show_bug.cgi?id=82201

Attachment 133802: Patch
https://bugs.webkit.org/attachment.cgi?id=133802&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=133802&action=review


> Source/WebCore/bindings/v8/V8Proxy.cpp:642
> +	   RefPtr<SharedPersistent<v8::Context> > context =
isolatedContext->sharedContext();

Why is this a RefPtr?

> Source/WebCore/bindings/v8/V8Proxy.h:248
> +	   // context() and persistentContext() return the same context.
> +	   // While context() returns a newly created Local handle,
> +	   // persistentContext() returns an existing Persistent handle.
>	   v8::Local<v8::Context> context();
> +	   v8::Handle<v8::Context> persistentContext();

I'm pretty sure we use a new local handle here because we're worried about our
persistent handle getting disposed while we've entered it.  Consider what
happens if we run page JavaScript while having entered our persistent handle
and that JavaScript causes the Frame and everything to be torn down.  We'll
dispose the persistent handle, which could cause the context to be destroyed.

I'm pretty sure we've had crashes like that before and that's why we use a new
local handle (to ensure that the context stays around).