Bug 111724

Summary: [V8] Store main world and non-main world templates separately.
Product: WebKit Reporter: Marja Hölttä <marja>
Component: New BugsAssignee: Marja Hölttä <marja>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, alecflett, dcarney, dglazkov, dgrogan, eric.carlson, feature-media-reviews, haraken, jamesr, japhet, jer.noble, jsbell, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 111968, 111971, 112136, 112182, 112262, 112444    
Bug Blocks: 110874    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Marja Hölttä 2013-03-07 06:33:54 PST
- 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.
Comment 1 Marja Hölttä 2013-03-07 08:35:43 PST
Created attachment 192006 [details]
Patch
Comment 2 jochen 2013-03-07 09:13:35 PST
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?
Comment 3 WebKit Review Bot 2013-03-07 11:20:34 PST
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
Comment 4 Adam Barth 2013-03-07 13:27:36 PST
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?
Comment 5 Kentaro Hara 2013-03-07 16:54:20 PST
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);
  }
Comment 6 Marja Hölttä 2013-03-11 01:49:34 PDT
.. 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.
Comment 7 Kentaro Hara 2013-03-11 01:52:49 PDT
(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.
Comment 8 Marja Hölttä 2013-03-12 04:05:52 PDT
Created attachment 192704 [details]
Patch
Comment 9 Marja Hölttä 2013-03-12 05:05:53 PDT
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.
Comment 10 Marja Hölttä 2013-03-12 08:06:53 PDT
Created attachment 192737 [details]
Patch
Comment 11 jochen 2013-03-12 08:14:44 PDT
Comment on attachment 192737 [details]
Patch

r=me
Comment 12 Marja Hölttä 2013-03-12 08:56:01 PDT
Created attachment 192747 [details]
Patch
Comment 13 WebKit Review Bot 2013-03-12 09:46:35 PDT
Comment on attachment 192747 [details]
Patch

Clearing flags on attachment: 192747

Committed r145554: <http://trac.webkit.org/changeset/145554>
Comment 14 WebKit Review Bot 2013-03-12 09:46:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 James Robinson 2013-03-12 13:06:23 PDT
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
Comment 16 WebKit Review Bot 2013-03-12 13:09:46 PDT
Re-opened since this is blocked by bug 112182
Comment 17 Marja Hölttä 2013-03-13 05:33:14 PDT
Created attachment 192905 [details]
Patch
Comment 18 Kentaro Hara 2013-03-13 06:02:56 PDT
(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?
Comment 19 Marja Hölttä 2013-03-13 06:19:13 PDT
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.
Comment 20 Kentaro Hara 2013-03-13 06:23:41 PDT
The change looks reasonable, but how about fixing bug 112250 first? Then you don't need to introduce GetFromContext, do you?
Comment 21 Marja Hölttä 2013-03-14 03:24:30 PDT
Created attachment 193101 [details]
Patch
Comment 22 Marja Hölttä 2013-03-14 03:26:46 PDT
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 23 jochen 2013-03-14 03:34:02 PDT
Comment on attachment 193101 [details]
Patch

rs=me
Comment 24 WebKit Review Bot 2013-03-14 04:14:42 PDT
Comment on attachment 193101 [details]
Patch

Clearing flags on attachment: 193101

Committed r145802: <http://trac.webkit.org/changeset/145802>
Comment 25 WebKit Review Bot 2013-03-14 04:14:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 WebKit Review Bot 2013-03-15 07:10:00 PDT
Re-opened since this is blocked by bug 112444
Comment 27 Marja Hölttä 2013-03-15 08:04:03 PDT
Created attachment 193310 [details]
Patch
Comment 28 Marja Hölttä 2013-03-15 08:07:01 PDT
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 29 jochen 2013-03-15 08:14:05 PDT
Comment on attachment 193310 [details]
Patch

rs=me
Comment 30 WebKit Review Bot 2013-03-15 08:39:47 PDT
Comment on attachment 193310 [details]
Patch

Clearing flags on attachment: 193310

Committed r145906: <http://trac.webkit.org/changeset/145906>
Comment 31 WebKit Review Bot 2013-03-15 08:39:53 PDT
All reviewed patches have been landed.  Closing bug.