Bug 111724 – [V8] Store main world and non-main world templates separately.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Marja Hölttä]

- Add the "world type" parameter to V8PerIsolateData::(rawT|t)emplateMap and store templates for non-main world in a separate map.
- Static FunctionTemplates inside functions need to be separated by world too.
- Currently, all the templates will be equal (the main world ones won't contain different function pointers than the non-main world ones). After the next step, we'll specialize the main world templates so that some of them will be different than the non-main world ones.

This is needed for bug 110874 and needs to be done before the patch in bug 111417.

[Marja Hölttä]

Created attachment 192006 [details]
Patch

Comment on attachment 192006 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192006&action=review

ok

> Source/WebCore/ChangeLog:30
> +        * bindings/v8/custom/V8LocationCustom.cpp:

can you add a comment why you only had to change these custom bindings?

[WebKit Review Bot]

Comment on attachment 192006 [details]
Patch

Attachment 192006 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17086329

New failing tests:
inspector/extensions/extensions-eval-content-script.html
inspector/extensions/extensions-audits-content-script.html

[Adam Barth]

Comment on attachment 192006 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192006&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:792
> +static v8::Handle<v8::Value> ${funcName}AttrGetterCallback(v8::Local<v8::String> name, const v8::AccessorInfo& info)
> +{
> +    if (worldType(info.GetIsolate()) == MainWorld)
> +        return ${interfaceName}V8Internal::${funcName}AttrGetterInMainWorld(name, info);
> +    return ${interfaceName}V8Internal::${funcName}AttrGetterInNonMainWorld(name, info);
> +}

I don't understand why we need this special case for domain safe functions.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:798
> +sub GenerateDomainSafeFunctionGetterHelper

"Helper" is not a very meaningful name for this function.  Is there a better name we can use?

[Kentaro Hara]

Comment on attachment 192006 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192006&action=review

I might want to see a couple of more iterations:)

> Source/WebCore/ChangeLog:13
> +        No new tests (no changes in functionality yet).

Please update run-bindings-tests.

>> Source/WebCore/ChangeLog:30
>> +        * bindings/v8/custom/V8LocationCustom.cpp:
> 
> can you add a comment why you only had to change these custom bindings?

The change looks reasonable, although I don't understand why these custom getters are needed in the first place. I tried to kill all of them, but couldn't due to test failures... (https://bugs.webkit.org/show_bug.cgi?id=109789)

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:784
> +    GenerateDomainSafeFunctionGetterHelper($function, $interfaceName, "InMainWorld", "MainWorld");
> +    GenerateDomainSafeFunctionGetterHelper($function, $interfaceName, "InNonMainWorld", "NonMainWorld");

Let's make naming consistent: "ForMainWorld" or "InMainWorld". You mix both in this patch. It looks like we prefer "ForMainWorld" in V8 bindings.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:-819
> -static v8::Handle<v8::Value> ${funcName}AttrGetterCallback(v8::Local<v8::String> name, const v8::AccessorInfo& info)
> -{
> -    return ${interfaceName}V8Internal::${funcName}AttrGetter(name, info);
> -}
> -

You shouldn't remove this. We're currently implementing wrapper methods for all the entry points to DOM custom/non-custom getters/setters/callbacks/constructors. The wrapper methods are named xxxCallback(). The purpose is to enable us to hook all DOM operations (e.g. logging, profiling etc).

Thus, here you can generate:

  static v8::Handle<v8::Value> ${funcName}AttrGetterForMainWorldCallback(v8::Local<v8::String> name, const v8::AccessorInfo& info)
  {
    return ${interfaceName}V8Internal::${funcName}AttrGetterForMainWorld(name, info);
  }

  static v8::Handle<v8::Value> ${funcName}AttrGetterCallback(v8::Local<v8::String> name, const v8::AccessorInfo& info)
  {
    return ${interfaceName}V8Internal::${funcName}AttrGetter(name, info);
  }

[Marja Hölttä]

.. I need to split up this patch, too, there are too many fixes to make this work.

Known needed fixes:
- "can access frame" check in (V8)InjectedScriptManager
- killing the static functiontemplate parameters inside generated functions (and V8LocationCustom)
- something has to be done to HasInstance (either check all worlds, or pass a world type)

With these fixes, the 2 failing tests,
inspector/extensions/extensions-eval-content-script.html
and
inspector/extensions/extensions-audits-content-script.html
will pass.

[Kentaro Hara]

(In reply to comment #6)
> Known needed fixes:
> - "can access frame" check in (V8)InjectedScriptManager
> - killing the static functiontemplate parameters inside generated functions (and V8LocationCustom)
> - something has to be done to HasInstance (either check all worlds, or pass a world type)
> 
> With these fixes, the 2 failing tests,
> inspector/extensions/extensions-eval-content-script.html
> and
> inspector/extensions/extensions-audits-content-script.html
> will pass.

Sounds good.

[Marja Hölttä]

Created attachment 192704 [details]
Patch

[Marja Hölttä]

Comment on attachment 192006 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192006&action=review

>> Source/WebCore/ChangeLog:13
>> +        No new tests (no changes in functionality yet).
> 
> Please update run-bindings-tests.

Done.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:784
>> +    GenerateDomainSafeFunctionGetterHelper($function, $interfaceName, "InNonMainWorld", "NonMainWorld");
> 
> Let's make naming consistent: "ForMainWorld" or "InMainWorld". You mix both in this patch. It looks like we prefer "ForMainWorld" in V8 bindings.

Fixed.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:792
>> +}
> 
> I don't understand why we need this special case for domain safe functions.

I simplified this, so we don't generate 2 functions (for main and non-main), but only one. That's not really needed in this step, it was just unnecessarily complication.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:798
>> +sub GenerateDomainSafeFunctionGetterHelper
> 
> "Helper" is not a very meaningful name for this function.  Is there a better name we can use?

This was removed.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:-819
>> -
> 
> You shouldn't remove this. We're currently implementing wrapper methods for all the entry points to DOM custom/non-custom getters/setters/callbacks/constructors. The wrapper methods are named xxxCallback(). The purpose is to enable us to hook all DOM operations (e.g. logging, profiling etc).
> 
> Thus, here you can generate:
> 
>   static v8::Handle<v8::Value> ${funcName}AttrGetterForMainWorldCallback(v8::Local<v8::String> name, const v8::AccessorInfo& info)
>   {
>     return ${interfaceName}V8Internal::${funcName}AttrGetterForMainWorld(name, info);
>   }
> 
>   static v8::Handle<v8::Value> ${funcName}AttrGetterCallback(v8::Local<v8::String> name, const v8::AccessorInfo& info)
>   {
>     return ${interfaceName}V8Internal::${funcName}AttrGetter(name, info);
>   }

I wasn't removing it, it was moved above. Anyway, this part of the code is simpler now.

[Marja Hölttä]

Created attachment 192737 [details]
Patch

Comment on attachment 192737 [details]
Patch

r=me

[Marja Hölttä]

Created attachment 192747 [details]
Patch

[WebKit Review Bot]

Comment on attachment 192747 [details]
Patch

Clearing flags on attachment: 192747

Committed r145554: <http://trac.webkit.org/changeset/145554>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

I believe this broke several ExtensionBrowserTests:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=@ToT - chromium.org&testType=browser_tests&tests=ExtensionBrowserTest.RSSMultiRelLink,ExecuteScriptApiTest.ExecuteScriptFrameAfterLoad,ExtensionBrowserTest.PageAction,ExecuteScriptApiTest.ExecuteScriptCallback

[WebKit Review Bot]

Re-opened since this is blocked by bug 112182

[Marja Hölttä]

Created attachment 192905 [details]
Patch

[Kentaro Hara]

(In reply to comment #17)
> Created an attachment (id=192905) [details]
> Patch

What's the difference from the previous patch? I mean, what is the cause of the ExtensionBrowserTests' failures?

[Marja Hölttä]

The difference is that that in this version hasInstance() checks from all worlds, and doesn't try to do things like
- try to determine the world type from the context
- only check templates from the given world type

This will "fix" the extension tests but only because it won't uncover some problems that the previous patch uncovered.

But I don't think we want to do that.

[Kentaro Hara]

The change looks reasonable, but how about fixing bug 112250 first? Then you don't need to introduce GetFromContext, do you?

[Marja Hölttä]

Created attachment 193101 [details]
Patch

[Marja Hölttä]

The latest patch
- undoes the ugly hasInstance hack and does the check the right way
- does not introduce GetFromContext world type; call sites need to pass the world type explicitly
- fixes the rest of the failing chromium tests by checking hasInstance for all worlds in WebArrayBuffer(View?)

Now that the patch in bug 112262 has landed, all the previously failing chromium browser_tests now pass.

Comment on attachment 193101 [details]
Patch

rs=me

[WebKit Review Bot]

Comment on attachment 193101 [details]
Patch

Clearing flags on attachment: 193101

Committed r145802: <http://trac.webkit.org/changeset/145802>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 112444

[Marja Hölttä]

Created attachment 193310 [details]
Patch

[Marja Hölttä]

Comment on attachment 193310 [details]
Patch

New try, since the landed version of this broke Chrome.

In the new try, we add HasInstanceInAnyWorld, and use that in WebBindings.cpp, and in the places where the previous patch had this multi-world HasInstance checking.

Comment on attachment 193310 [details]
Patch

rs=me

[WebKit Review Bot]

Comment on attachment 193310 [details]
Patch

Clearing flags on attachment: 193310

Committed r145906: <http://trac.webkit.org/changeset/145906>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.