Bug 27418 – [V8] Factor V8ContextFactory out of V8Proxy

• Component: WebCore JavaScript
• Status: RESOLVED LATER

[Adam Barth]

Patch forthcoming.

[Adam Barth]

Created attachment 33031 [details]
patch

[Adam Barth]

Created attachment 33032 [details]
patch

[Adam Barth]

Created attachment 33033 [details]
patch

[Adam Barth]

Ok.  That's better.  There are some static initializers in this patch that
existed before.  I'll remove them in a followup patch.

[David Levin]

Comment on attachment 33033 [details]
patch

I tried to review this but the re-ordering of operations doesn't make this as mechanical as I would like (for me to review), so I'll just note the things I saw during my attempt and not do an r+ or r-.


> Index: WebCore/bindings/v8/V8ContextFactory.cpp
> +#include "config.h"
> +#include "Frame.h"
> +#include "DocumentLoader.h"
> +#include "DOMWindow.h"
> +#include "Page.h"

> +#include "V8ContextFactory.h"

This one should come first after config.h


> +v8::Persistent<v8::Context> V8ContextFactory::create(Frame* frame, v8::Handle<v8::Object> global)
> +{

> +    // Enter the next context.
> +    v8::Context::Scope contextScope(context);
> +
> +    if (!installHiddenObjectPrototype(context) || !installDOMWindow(context, frame->domWindow())) {
> +        context.Dispose();
> +        context.Clear();
> +    }

This change made it hardest for me to review because (to my v8 naive eyes) it look like this may cause different code to be executed in some cases.

For example, previously it check  if (context.IsEmpty()) before doing this.
It also did the code in "// Store the first global object created so we can reuse " before doing this.  Now this code is executed first and may clear the context so that the "global object" code may not be executed.
It may all be correct, but it was the least mechanical part of the changes so I couldn't easily determine its correctness.



> +bool V8ContextFactory::installDOMWindow(v8::Handle<v8::Context> context, DOMWindow* window)
...
> +    // Create a new JS window object and use it as the prototype for the  shadow global object.

It was there before but please get rid of the double space before shadow.


> +void V8ContextFactory::ensureUtilityContext()
> +{
> +    if (!m_utilityContext.IsEmpty())
> +        return;

This was an assert before. Why is it changing?

> Index: WebCore/bindings/v8/V8ContextFactory.h
> +#include <v8.h>

I think it may be standard at this point to put this header first in these files but it goes against the standard ordering of these headers, so why not (fix this and) move it to where the other <> headers are?


> +        // the entire process. Plan accoringly.
typo: accoringly 

> +}

Change to "} // namespace WebCore"

> Index: WebCore/bindings/v8/V8Proxy.cpp

> @@ -479,9 +466,6 @@ void V8Proxy::evaluateInNewContext(const
>  
>      v8::Handle<v8::Object> global = context->Global();
>  
> -    v8::Handle<v8::String> implicitProtoString = v8::String::New("__proto__");
> -    global->Set(implicitProtoString, windowWrapper);
> -

Why is it ok to remove this? (it feels unrelated to the rest of your change.)

> Index: WebCore/bindings/v8/V8Proxy.h
> +        static void reportUnsafeJavaScriptAccess(v8::Local<v8::Object> host, v8::AccessType type, v8::Local<v8::Value> data);

It doesn't look like the param name "type" adds any information.  Remove it?

[Adam Barth]

(In reply to comment #5)
> I tried to review this but the re-ordering of operations doesn't make this as
> mechanical as I would like (for me to review), so I'll just note the things I
> saw during my attempt and not do an r+ or r-.

Yes.  This one is much more complex than the other ones.  I tried to do this in two steps, but this seemed easier.  I can try to break it up again...

> > +#include "V8ContextFactory.h"
> 
> This one should come first after config.h

Fixed.

> > +v8::Persistent<v8::Context> V8ContextFactory::create(Frame* frame, v8::Handle<v8::Object> global)
> > +{
> 
> > +    // Enter the next context.
> > +    v8::Context::Scope contextScope(context);
> > +
> > +    if (!installHiddenObjectPrototype(context) || !installDOMWindow(context, frame->domWindow())) {
> > +        context.Dispose();
> > +        context.Clear();
> > +    }
> 
> This change made it hardest for me to review because (to my v8 naive eyes) it
> look like this may cause different code to be executed in some cases.

Yes.  This changes the code path slightly.

> For example, previously it check  if (context.IsEmpty()) before doing this.

Yes.  I think that was missing in the new version.  Re-added.

> It also did the code in "// Store the first global object created so we can
> reuse " before doing this.  Now this code is executed first and may clear the
> context so that the "global object" code may not be executed.
> It may all be correct, but it was the least mechanical part of the changes so I
> couldn't easily determine its correctness.

I think this part is fine.  The code isn't written that well, but the intent is for the creation function to be transactional.  If the context creating succeed, great.  If not, the changes are supposed to be rolled back via disposeContextHandles(), which, for example, clears out any m_global we might have stored.

It's possible I should do another round to make the transactional nature more clear / robust to changes.

> > +bool V8ContextFactory::installDOMWindow(v8::Handle<v8::Context> context, DOMWindow* window)
> ...
> > +    // Create a new JS window object and use it as the prototype for the  shadow global object.
> 
> It was there before but please get rid of the double space before shadow.

Fixed.

> > +void V8ContextFactory::ensureUtilityContext()
> > +{
> > +    if (!m_utilityContext.IsEmpty())
> > +        return;
> 
> This was an assert before. Why is it changing?

Because now we call this function unconditionally instead of putting the branch in the caller.  This seems more robust.

> > Index: WebCore/bindings/v8/V8ContextFactory.h
> > +#include <v8.h>
> 
> I think it may be standard at this point to put this header first in these
> files but it goes against the standard ordering of these headers, so why not
> (fix this and) move it to where the other <> headers are?

I'm not sure exactly what order your asking for, but I've changed this to be more like other files.  Let me know if this isn't what you have in mind.

> > +        // the entire process. Plan accoringly.
> typo: accoringly 

Fixed.

> 
> > +}
> 
> Change to "} // namespace WebCore"

Fixed.

> > -    v8::Handle<v8::String> implicitProtoString = v8::String::New("__proto__");
> > -    global->Set(implicitProtoString, windowWrapper);
> > -
> 
> Why is it ok to remove this? (it feels unrelated to the rest of your change.)

Basically, this code was doing installDOMWindow manually before.  By changing this code to call V8ContextFactory::create, it get the same installDOMWindow codepath as everyone else.  That means it doesn't need to do this __proto__ hack because the hack is centralized in in installDOMWindow.

> > Index: WebCore/bindings/v8/V8Proxy.h
> > +        static void reportUnsafeJavaScriptAccess(v8::Local<v8::Object> host, v8::AccessType type, v8::Local<v8::Value> data);
> 
> It doesn't look like the param name "type" adds any information.  Remove it?

Fixed.

[Adam Barth]

Created attachment 33072 [details]
patch

[Adam Barth]

Comment on attachment 33072 [details]
patch

Clearing the review flag because this patch no longer applies cleanly to TOT.

[Adam Barth]

We've gotten V8Proxy into a reasonable state.  We can do the rest later.