[webkit-reviews] review denied: [Bug 40162] Prevent Geolocation making callbacks to a ScriptExecutionContext that no longer exists : [Attachment 57984] Patch

[bugzilla-daemon at webkit.org]

Jeremy Orlow <jorlow at chromium.org> has denied Steve Block
<steveblock at google.com>'s request for review:
Bug 40162: Prevent Geolocation making callbacks to a ScriptExecutionContext
that no longer exists
https://bugs.webkit.org/show_bug.cgi?id=40162

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

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
WebCore/bindings/v8/custom/V8GeolocationCustom.cpp:59
 +	Frame* frame = V8Proxy::retrieveFrameForCurrentContext();
Just get the current context directly.

WebCore/bindings/v8/custom/V8GeolocationCustom.cpp:78
 +	Frame* frame = V8Proxy::retrieveFrameForCurrentContext();
ditto

WebCore/bindings/v8/custom/V8CustomPositionCallback.cpp:72
 +	// Protect the script context until the callback returns.
Are you sure we need one of these?

WebCore/bindings/js/JSCustomPositionErrorCallback.cpp:48
 +	// ActiveDOMObject will null our pointer to the ScriptExecutionContext
when
I'd lean towards not wrapping this comment.

WebCore/bindings/js/JSCustomPositionErrorCallback.cpp:50
 +	if (!scriptExecutionContext())
This is a good start, but ideally you'd be handling resume/suspend/stop instead
of just detecting when the scriptExecutionContext has been destructed.

WebCore/ChangeLog:16
 +	    The ScriptExecutionContext is ref'ed from script, so isn't
destroyed until the
so _it_ isn't...

WebCore/ChangeLog:19
 +	    accessing the Frame, so an additional check for the Frame is
required.
Is any of this still relevant?

Overall, I think this change log description could be made more concise and not
lose any interesting info.

WebCore/ChangeLog:21
 +	    This change also prevents the V8 bindings from incorrectly holding
a reference to the Frame.
We need to make sure this gets fixed in the other bindings.  Maybe hans or dumi
would be interested in this, if you're not?