Bug 34658 - [V8] Use toV8() for wrapping objects in a WorkerContext
Summary: [V8] Use toV8() for wrapping objects in a WorkerContext
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-05 13:09 PST by Nate Chapin
Modified: 2010-02-08 11:47 PST (History)
0 users

See Also:


Attachments
patch (19.52 KB, patch)
2010-02-05 13:42 PST, Nate Chapin
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2010-02-05 13:09:50 PST
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.
Comment 1 Nate Chapin 2010-02-05 13:42:41 PST
Created attachment 48254 [details]
patch

Perf tests are still running, but I think this change is ok.
Comment 2 Dimitri Glazkov (Google) 2010-02-06 09:45:17 PST
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?
Comment 3 Nate Chapin 2010-02-08 09:23:09 PST
(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 :)
Comment 4 Dimitri Glazkov (Google) 2010-02-08 09:45:16 PST
Comment on attachment 48254 [details]
patch

ok. I trust you'll monitor the perf bots.
Comment 5 Nate Chapin 2010-02-08 11:47:09 PST
http://trac.webkit.org/changeset/54499