Bug 107020 - [V8] Make a creationContext parameter of toV8() mandatory
Summary: [V8] Make a creationContext parameter of toV8() mandatory
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-16 08:51 PST by Kentaro Hara
Modified: 2013-01-16 11:23 PST (History)
10 users (show)

See Also:


Attachments
Patch (43.43 KB, patch)
2013-01-16 08:55 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2013-01-16 08:51:15 PST
Currently a creationContext parameter of toV8() is optional:

  Handle<Value> toV8(..., Handle<Object> creationContext = Handle<Object>(), ...) {
    ...;
  }

On the other hand, looking at the implementation of V8WrapperInstantiationScope:

    explicit V8WrapperInstantiationScope(v8::Handle<v8::Object> creationContext)
        : m_didEnterContext(false)
        , m_context(v8::Context::GetCurrent())
    {
        if (creationContext.IsEmpty()) // We do nothing for this case!
            return;
        v8::Handle<v8::Context> contextForWrapper = creationContext->CreationContext();
        if (contextForWrapper == m_context)
            return;
        m_context = v8::Local<v8::Context>::New(contextForWrapper);
        m_didEnterContext = true;
        m_context->Enter();
    }

we're assuming that a creationContext parameter is passed to toV8() when a creationContext exists. In other words, if we forget to pass a creationContext parameter, we will end up with creating an object on the current context, which is wrong. To avoid the mistake, we should make the creationContext parameter mandatory. When we really do not have a creationContext, we can pass Handle<Object>() explicitly.
Comment 1 Kentaro Hara 2013-01-16 08:55:51 PST
Created attachment 182993 [details]
Patch
Comment 2 Adam Barth 2013-01-16 10:59:38 PST
Comment on attachment 182993 [details]
Patch

Yeah, we should really pass non-empty handles in all these places.
Comment 3 WebKit Review Bot 2013-01-16 11:23:06 PST
Comment on attachment 182993 [details]
Patch

Clearing flags on attachment: 182993

Committed r139900: <http://trac.webkit.org/changeset/139900>
Comment 4 WebKit Review Bot 2013-01-16 11:23:10 PST
All reviewed patches have been landed.  Closing bug.