Bug 34658 – [V8] Use toV8() for wrapping objects in a WorkerContext

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nate Chapin]

The main thing this requires is checking for the existence of a WorkerContextExecutionProxy in V8DOMWrapper::instantiateV8Object() prior to checking for a V8Proxy, since calling V8Proxy::retrieve() in a WorkerContext will crash us.

[Nate Chapin]

Created attachment 48254 [details]
patch

Perf tests are still running, but I think this change is ok.

[Dimitri Glazkov (Google)]

Comment on attachment 48254 [details]
patch

This looks great! Except:

> +    // Get the WorkerContextExecutionProxy first. If we are in a WorkerContext and we try to call V8Proxy::retrieve(),
> +    // we crash trying to retrieve a DOMWindow.
> +    WorkerContextExecutionProxy* workerContextProxy = WorkerContextExecutionProxy::retrieve();

Are you sure this doesn't affect performance?

[Nate Chapin]

(In reply to comment #2)
> (From update of attachment 48254 [details])
> This looks great! Except:
> 
> > +    // Get the WorkerContextExecutionProxy first. If we are in a WorkerContext and we try to call V8Proxy::retrieve(),
> > +    // we crash trying to retrieve a DOMWindow.
> > +    WorkerContextExecutionProxy* workerContextProxy = WorkerContextExecutionProxy::retrieve();
> 
> Are you sure this doesn't affect performance?

I ran the dromaeo tests locally and the results were comparable with and without the change.  If there's anything else you'd like me to run, please let me know :)

[Dimitri Glazkov (Google)]

Comment on attachment 48254 [details]
patch

ok. I trust you'll monitor the perf bots.

[Nate Chapin]

http://trac.webkit.org/changeset/54499