Apply script preprocessor to Web page scripts only.

Apply ScriptPreprocessor to Web page scripts only.
    
        Move ScriptPreprocessor from ScriptDebugServer to ScriptController.
        Evaluate preprocessor in isolatedWorld.
        Preprocess event listeners and <script> elements before entering V8.
        Preprocess eval source in V8 beforeCompile event.
    
        Add Debugger.getScriptCompilationTypeInfo() to extract the
        V8 compilation script info, to seperate eval scripts from
        e.g. devtools command line evaluations.
    
        Report exceptions in the ScriptPreprocessor compilation.
        Add new test-cases for these error paths.
    
        BUG=226293

[johnjbarton, 2013-04-04 18:03:21]

Ready for review

[johnjbarton, 2013-04-05 15:37:00]

Try 2

[johnjbarton, 2013-04-30 22:30:04]

Greg Simon suggested haraken as a reviewer as well.

[pfeldman, 2013-05-01 07:33:55]

I am not a fan of the complexity it introduces given that the only client is an
extension. Like, you suggest to bail out if:

the isSystemScope is set. 
script name is "data:text/html,chromewebdata". 
contextCategory is 'injected' 

I'd suggest narrowing it down to isSystemScope. Things like chromewebdata will
come and go.

Also, SystemScope clashes with Context::Scope a bit and I don't think this is
intentional. You also introduce contextCategory that is somewhat related. I'm in
favor of simplifying things prior to landing such a change + explaining how it
could be used in Chrome itself.

https://codereview.chromium.org/13575004/diff/16001/Source/bindings/v8/Script...
File Source/bindings/v8/ScriptDebugServer.cpp (right):

https://codereview.chromium.org/13575004/diff/16001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptDebugServer.cpp:470: if
(V8PerContextDebugData::isSystemScope(eventContext) ||
V8PerContextDebugData::contextCategory(eventContext) == "injected")
Please introduce constant for this so that we could track where it is set. Also,
can we make injected contexts have system scope instead?

https://codereview.chromium.org/13575004/diff/16001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptDebugServer.cpp:487: if
(scriptNameString.contains("data:text/html,chromewebdata"))
You can't say "chromewebdata" from blink which is presumable Chrome-unaware.
Same thing, can it be system?

https://codereview.chromium.org/13575004/diff/16001/Source/bindings/v8/V8PerC...
File Source/bindings/v8/V8PerContextData.cpp (right):

https://codereview.chromium.org/13575004/diff/16001/Source/bindings/v8/V8PerC...
Source/bindings/v8/V8PerContextData.cpp:213: static
V8PerContextDebugData::SystemScope* currentSystemScope = 0;
static scope is likely to be non-workers friendly (they are running on a
different thread)

https://codereview.chromium.org/13575004/diff/16001/Source/bindings/v8/V8PerC...
Source/bindings/v8/V8PerContextData.cpp:215: bool
V8PerContextDebugData::isSystemScope(v8::Handle<v8::Context> context)
isSystemScope for Context does not correlate with v8::Context::Scope, right?

[johnjbarton, 2013-05-01 15:45:53]

On 2013/05/01 07:33:55, pfeldman wrote:
> I'd suggest narrowing it down to isSystemScope. Things like chromewebdata will
> come and go.

Perhaps we could use a positive test rather than a negative one. By that I mean,
eg isWebCompilationScope, which would be true if and only if the compilation was
from a web page or worker.  That should reduce the potential variability.  What
do you think about that approach?

> 
> Also, SystemScope clashes with Context::Scope a bit and I don't think this is
> intentional.
...https://codereview.chromium.org/13575004/diff/16001/Source/bindings/v8/V8PerContextData.cpp#newcode215
> Source/bindings/v8/V8PerContextData.cpp:215: bool
> V8PerContextDebugData::isSystemScope(v8::Handle<v8::Context> context)
> isSystemScope for Context does not correlate with v8::Context::Scope, right?

The isSystemScope setting lifetime is less than or equal to the
v8::Context::Scope. The 'Scope' word is meant to indicate that the SystemScope
object lifetime controls the setting, the same way that v8::Context::Scope
works.  

Does that answer your question? Would SystemCompilationScope be better?  Or
ScopedSystemCompilation by analogy to WebScopedMicrotaskSuppression?


>
https://codereview.chromium.org/13575004/diff/16001/Source/bindings/v8/V8PerC...
> Source/bindings/v8/V8PerContextData.cpp:213: static
> V8PerContextDebugData::SystemScope* currentSystemScope = 0;
> static scope is likely to be non-workers friendly (they are running on a
> different thread)

All we need is for the creation of the SystemScope object to occur on the same
thread as the debugListener for the beforeCompile event. As far as I understand,
this is always true.  This SystemScope is to compilation as
WebScopedMicrotaskSuppression is to running scripts.

[johnjbarton, 2013-05-03 22:38:43]

PTAL,

On 2013/05/01 07:33:55, pfeldman wrote:
> I am not a fan of the complexity it introduces given that the only client is
an
> extension. Like, you suggest to bail out if:
> 
> the isSystemScope is set. 
> script name is "data:text/html,chromewebdata". 
> contextCategory is 'injected' 
> 
> I'd suggest narrowing it down to isSystemScope. Things like chromewebdata will
> come and go.

I reversed the sense of the filtering scheme, setting the mark when 
we are operating on Web scripts rather than not. That reduces the variability.

> 
> Also, SystemScope clashes with Context::Scope a bit and I don't think this is
> intentional.

In addition to reversing the filtering, I changed the name:
ScopedWebCompilation.
 
> You also introduce contextCategory that is somewhat related. 

My ContextCategory (now an enum, see below) is just a name
for the pre-existing V8 Context-embedded data. That data was eg 'page, 2' or
'injected, 2', where the string indicates the purpose of the
Context.

The ScopedWebCompilation tells us that not only are we in a web page
context, but in addition we are compiling web scripts, not extension API
scripts, console-injection scripts, or command line scripts.

> I'm in
> favor of simplifying things prior to landing such a change + explaining how it
> could be used in Chrome itself.

This new approach does not require changes in Chrome.

> 
>
https://codereview.chromium.org/13575004/diff/16001/Source/bindings/v8/Script...
> File Source/bindings/v8/ScriptDebugServer.cpp (right):
> 
>
https://codereview.chromium.org/13575004/diff/16001/Source/bindings/v8/Script...
> Source/bindings/v8/ScriptDebugServer.cpp:470: if
> (V8PerContextDebugData::isSystemScope(eventContext) ||
> V8PerContextDebugData::contextCategory(eventContext) == "injected")
> Please introduce constant for this so that we could track where it is set. 

I changed contextCategory to return an enum. The string "injected"
is used in the code currently, we could remove it in another bug 
easily.

> Also,
> can we make injected contexts have system scope instead?

I removed the system scope. 

> 
>
https://codereview.chromium.org/13575004/diff/16001/Source/bindings/v8/Script...
> Source/bindings/v8/ScriptDebugServer.cpp:487: if
> (scriptNameString.contains("data:text/html,chromewebdata"))
> You can't say "chromewebdata" from blink which is presumable Chrome-unaware.
> Same thing, can it be system?

Solved by the new patch as well.

> 
>
https://codereview.chromium.org/13575004/diff/16001/Source/bindings/v8/V8PerC...
> File Source/bindings/v8/V8PerContextData.cpp (right):
> 
>
https://codereview.chromium.org/13575004/diff/16001/Source/bindings/v8/V8PerC...
> Source/bindings/v8/V8PerContextData.cpp:213: static
> V8PerContextDebugData::SystemScope* currentSystemScope = 0;
> static scope is likely to be non-workers friendly (they are running on a
> different thread)

Turns out I don't think Workers be preprocessed yet, see Bug 237900.

As long as the ScopedWebCompilation object is on the same thread as
the compile, we should not have a problem. We are setting a global
just before entering the V8 and testing it just on the other
side (beforeCompile). 

However, I'm not keen on this solution. I think the right solution
is to set the V8 embedding data before every compile so the debugger
knows why the compile/run was triggered. This will also mark the
script object via the transfer in V8 MakeFunctionInfo. But that is 
a much larger patch and the current solution is adequate for preprocessing.

> 
>
https://codereview.chromium.org/13575004/diff/16001/Source/bindings/v8/V8PerC...
> Source/bindings/v8/V8PerContextData.cpp:215: bool
> V8PerContextDebugData::isSystemScope(v8::Handle<v8::Context> context)
> isSystemScope for Context does not correlate with v8::Context::Scope, right?

Hopefully I have clarified the new ScopedWebCompilation above.
Here is another version: We have lots of JS coming thru V8 + debugger.
We mark the JS with two properties: the context category (page,
injected, worker) and page number (debugId). Two more properties are
important to the preprocessor: who is sending the JS (eg page, debugger,
extension-system) and is the compilation constrained (eg event handlers must be
one function). Getting those two properties
to the ScriptDebugListener is the purpose of this patch.

[pfeldman, 2013-05-29 15:03:52]

https://codereview.chromium.org/13575004/diff/24001/Source/bindings/v8/Script...
File Source/bindings/v8/ScriptController.h (right):

https://codereview.chromium.org/13575004/diff/24001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptController.h:99: // Evaluate Web-origin JavaScript in
the main world. (Not for system uses)
Insert blank line above the comment. I think this brings in confusion on the
caller side. Now every caller needs to think whether its script it
"FromWebPage".

https://codereview.chromium.org/13575004/diff/24001/Source/core/dom/ScriptEle...
File Source/core/dom/ScriptElement.cpp (right):

https://codereview.chromium.org/13575004/diff/24001/Source/core/dom/ScriptEle...
Source/core/dom/ScriptElement.cpp:313:
frame->script()->evaluateFromWebPage(sourceCode);
Do we know of entry points other than script and DOM0? What happens to eval and
Function constructor? Do they end up "from web"?

[johnjbarton, 2013-05-29 23:45:12]

On 2013/05/29 15:03:52, pfeldman wrote:
...
> 
>
https://codereview.chromium.org/13575004/diff/24001/Source/core/dom/ScriptEle...
> File Source/core/dom/ScriptElement.cpp (right):
> 
>
https://codereview.chromium.org/13575004/diff/24001/Source/core/dom/ScriptEle...
> Source/core/dom/ScriptElement.cpp:313:
> frame->script()->evaluateFromWebPage(sourceCode);
> Do we know of entry points other than script and DOM0? What happens to eval
and
> Function constructor? Do they end up "from web"?

A follow up patch is needed for eval. Basically we need to allow preprocessing
of any eval that appears in the page context, since internal scripts don't
happen to eval and no-one else can. The follow-up patch extracts the V8 flag
marking evals (CompilationTypeInfo).

I could combine them here, let me know.

[abarth-chromium, 2013-06-04 06:50:23]

https://codereview.chromium.org/13575004/diff/33002/Source/bindings/v8/Script...
File Source/bindings/v8/ScriptController.cpp (right):

https://codereview.chromium.org/13575004/diff/33002/Source/bindings/v8/Script...
Source/bindings/v8/ScriptController.cpp:726: static const String
preprocessorName = "$preprocessor.js";
This will create a static initializer, which isn't allowed.

https://codereview.chromium.org/13575004/diff/33002/Source/bindings/v8/Script...
Source/bindings/v8/ScriptController.cpp:736: v8::Local<v8::Context> context =
v8::Context::New(m_isolate);
Hum...  I usually push back on calls to v8::Context::New.  We make a number of
assumptions about how contexts are set up, and ah-hoc contexts aren't going to
be set up the way we want.

https://codereview.chromium.org/13575004/diff/33002/Source/bindings/v8/Script...
Source/bindings/v8/ScriptController.cpp:744: v8::Handle<v8::String> preprocessor
= v8::String::New(preprocessorScript.utf8().data(),
preprocessorScript.utf8().length());
This isn't the right way to create v8 string from a WTFString.  That's why we
have the v8String function.

https://codereview.chromium.org/13575004/diff/33002/Source/bindings/v8/Script...
Source/bindings/v8/ScriptController.cpp:792: v8::Local<v8::Context> context =
v8::Local<v8::Context>::New(m_utilityContext.get());
I think we have a new pattern for this sort of code that avoids calling the
v8::Persistent copy constructor.  We're trying to get rid of all calls to the
v8::Persistent copy constructor.

https://codereview.chromium.org/13575004/diff/33002/Source/bindings/v8/Script...
Source/bindings/v8/ScriptController.cpp:811: return String::fromUTF8(*utf8Value,
utf8Value.length());
This isn't the way we create WTFStrings from v8::Strings.

[johnjbarton, 2013-06-04 18:41:22]

Marja, please review my use of ScopedPersistent for two members of
ScriptController::ScriptPreprocessor. (Of course any other suggestions would be
welcome).

https://codereview.chromium.org/13575004/diff/33002/Source/bindings/v8/Script...
File Source/bindings/v8/ScriptController.cpp (right):

https://codereview.chromium.org/13575004/diff/33002/Source/bindings/v8/Script...
Source/bindings/v8/ScriptController.cpp:726: static const String
preprocessorName = "$preprocessor.js";
On 2013/06/04 06:50:23, abarth wrote:
> This will create a static initializer, which isn't allowed.

Done.

https://codereview.chromium.org/13575004/diff/33002/Source/bindings/v8/Script...
Source/bindings/v8/ScriptController.cpp:736: v8::Local<v8::Context> context =
v8::Context::New(m_isolate);
On 2013/06/04 06:50:23, abarth wrote:
> Hum...  I usually push back on calls to v8::Context::New.  We make a number of
> assumptions about how contexts are set up, and ah-hoc contexts aren't going to
> be set up the way we want.

I don't know what to do here: what context shall I use?

https://codereview.chromium.org/13575004/diff/33002/Source/bindings/v8/Script...
Source/bindings/v8/ScriptController.cpp:744: v8::Handle<v8::String> preprocessor
= v8::String::New(preprocessorScript.utf8().data(),
preprocessorScript.utf8().length());
On 2013/06/04 06:50:23, abarth wrote:
> This isn't the right way to create v8 string from a WTFString.  That's why we
> have the v8String function.

Done.

https://codereview.chromium.org/13575004/diff/33002/Source/bindings/v8/Script...
Source/bindings/v8/ScriptController.cpp:792: v8::Local<v8::Context> context =
v8::Local<v8::Context>::New(m_utilityContext.get());
On 2013/06/04 06:50:23, abarth wrote:
> I think we have a new pattern for this sort of code that avoids calling the
> v8::Persistent copy constructor.  We're trying to get rid of all calls to the
> v8::Persistent copy constructor.

I've changed this code and I will ask Marja to review the use of
ScopedPersistent.

https://codereview.chromium.org/13575004/diff/33002/Source/bindings/v8/Script...
Source/bindings/v8/ScriptController.cpp:811: return String::fromUTF8(*utf8Value,
utf8Value.length());
On 2013/06/04 06:50:23, abarth wrote:
> This isn't the way we create WTFStrings from v8::Strings.

Done.

[abarth-chromium, 2013-06-04 20:07:00]

On 2013/06/04 18:41:22, johnjbarton wrote:
> I don't know what to do here: what context shall I use?

Why do we need to execute JavaScript at all?

[johnjbarton, 2013-06-04 21:41:22]

On 2013/06/04 20:07:00, abarth wrote:
> On 2013/06/04 18:41:22, johnjbarton wrote:
> > I don't know what to do here: what context shall I use?
> 
> Why do we need to execute JavaScript at all?

The preprocessor is a JS program, eg traceur.

[abarth-chromium, 2013-06-04 21:57:02]

On 2013/06/04 21:41:22, johnjbarton wrote:
> On 2013/06/04 20:07:00, abarth wrote:
> > On 2013/06/04 18:41:22, johnjbarton wrote:
> > > I don't know what to do here: what context shall I use?
> > 
> > Why do we need to execute JavaScript at all?
> 
> The preprocessor is a JS program, eg traceur.

I guess I'm missing context for the design of this feature.  This CL doesn't
even say what the feature is.  Is there a design doc I can read?  Are we
planning to bundle the traceur compiler with Chrome?  Is this some sort of
extension?

Rather than accepting CLs for this feature, we should first decide whether the
benefits of the feature outweigh the implementation costs.  If you need to spin
up yet-another-context that's not configured the way we expect contexts to be
configured, that's a big ongoing implementation cost.

[johnjbarton, 2013-06-04 22:29:53]

On 2013/06/04 21:57:02, abarth wrote:
> On 2013/06/04 21:41:22, johnjbarton wrote:
> > On 2013/06/04 20:07:00, abarth wrote:
> > > On 2013/06/04 18:41:22, johnjbarton wrote:
> > > > I don't know what to do here: what context shall I use?
> > > 
> > > Why do we need to execute JavaScript at all?
> > 
> > The preprocessor is a JS program, eg traceur.
> 
> I guess I'm missing context for the design of this feature.  This CL doesn't
> even say what the feature is.  Is there a design doc I can read?  Are we
> planning to bundle the traceur compiler with Chrome?  Is this some sort of
> extension?

This CL is a refactoring of the existing preprocessor feature. The preprocessor
source code is loaded from a devtools extension using 
   chrome.devtools.inspectedWindow.reload(reloadOptions);
with reloadOptions.preprocessor property. The preprocessor runs only if devtools
is open and only if the the devtools extension reloads the page with the 
preprocessor script.  

> 
> Rather than accepting CLs for this feature, we should first decide whether the
> benefits of the feature outweigh the implementation costs.  If you need to
spin
> up yet-another-context that's not configured the way we expect contexts to be
> configured, that's a big ongoing implementation cost.

Is there some way for me to find out what you expect for the context
configuration?

The context will only be created if devtools is open and the preprocessor is
installed.
The context just needs to be an empty JS environment with built-in function
support.
There is nothing fancy needed here. We just compile one function and call it.

I guess we could use v8::Debug::GetDebugContext(). On the plus side, it means
not creating
a context; on the minus side, mistakes in the preprocessor code could, in some
wild cases,
interfere with the debugger. Based on the V8 docs, creating contexts seems easy
and cheap,
so the new-context path wins. If there is more to it, perhaps we should consider
the 
other path.  However, I'm just guessing.

[abarth-chromium, 2013-06-04 23:02:10]

On 2013/06/04 22:29:53, johnjbarton wrote:
> This CL is a refactoring of the existing preprocessor feature. The
preprocessor
> source code is loaded from a devtools extension using 

What context does the script run in currently?

> Is there some way for me to find out what you expect for the context
> configuration?

See V8DOMWindowShell::initializeIfNeeded and V8DOMWindowShell::createContext.

> The context will only be created if devtools is open and the preprocessor is
> installed.

That's even worse.  That means this case is rare and obscure, which means code
won't understand it and will assume that it's interacting with a normal context.

> The context just needs to be an empty JS environment with built-in function
> support.

I understand.  The problem is all the other code in the system that calls
v8::Context::GetCurrent() or the like and assumes that the returned context is
configured in a certain way.

> There is nothing fancy needed here. We just compile one function and call it.

I understand that's your perspective.  My perspective is that this is one
component in a larger system, and this component doesn't respect the invariants
that the large system maintains.  That means this component is going to be a
source of bugs / crashes / security issues and generally make the entire system
more difficult to maintain.

> Based on the V8 docs, creating contexts seems easy and cheap,

That might be what the docs say, but it's not true.

My concern is more with the complexity introduced by creating
yet-another-context, especially one that's ad-hoc and doesn't respect the
invariants of the larger system.

You still haven't answered my previous question about what feature this code
supports and whether we've made a decision comparing the benefits the feature
brings to the costs of supporting the feature.  That's an important decision to
make before landing this sort of complexity-inducing CL.

[johnjbarton, 2013-06-05 00:01:35]

On 2013/06/04 23:02:10, abarth wrote:
> On 2013/06/04 22:29:53, johnjbarton wrote:
> > This CL is a refactoring of the existing preprocessor feature. The
> preprocessor
> > source code is loaded from a devtools extension using 
> 
> What context does the script run in currently?

m_utilityContext:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

> 
> > Is there some way for me to find out what you expect for the context
> > configuration?
> 
> See V8DOMWindowShell::initializeIfNeeded and V8DOMWindowShell::createContext.

Those functions do lots of things with lots of potential requirements and
complex implications. They are not needed for the preprocessor. 

> 
> > The context will only be created if devtools is open and the preprocessor is
> > installed.
> 
> That's even worse.  That means this case is rare and obscure, which means code
> won't understand it and will assume that it's interacting with a normal
context.
> 
> > The context just needs to be an empty JS environment with built-in function
> > support.
> 
> I understand.  The problem is all the other code in the system that calls
> v8::Context::GetCurrent() or the like and assumes that the returned context is
> configured in a certain way.

As far as I understand the code, the debugger is the only part of the system
active while the preprocessor context is active.

We enter the context, compile/run the preprocessor and exit the context; later
we enter the context, run the preprocessor over the source and exit the context.
 The context has no 'window' object and no connection to a Frame. So I believe
the V8 events are the only outlets.

> 
> > There is nothing fancy needed here. We just compile one function and call
it.
> 
> I understand that's your perspective.  My perspective is that this is one
> component in a larger system, and this component doesn't respect the
invariants
> that the large system maintains.  That means this component is going to be a
> source of bugs / crashes / security issues and generally make the entire
system
> more difficult to maintain.
> 
> > Based on the V8 docs, creating contexts seems easy and cheap,
> 
> That might be what the docs say, but it's not true.
> 
> My concern is more with the complexity introduced by creating
> yet-another-context, especially one that's ad-hoc and doesn't respect the
> invariants of the larger system.

I don't think the larger system even knows or needs to know about this context.
I agree that the context in V8DOMWindowShell is a seriously complex object and
if we were creating anything similar I would share your concerns. However, this
preprocessor context is not so much 'ad-hoc' as 'trivial', it has no connection
to the system other than the few bits of code in the ScriptPreprocessor itself.

> 
> You still haven't answered my previous question about what feature this code
> supports and whether we've made a decision comparing the benefits the feature
> brings to the costs of supporting the feature.  That's an important decision
to
> make before landing this sort of complexity-inducing CL.

As I said previously, the feature already exists, I'm just trying to make it
work properly.

This preprocessor allows us to transcode to JavaScript directly within the
browser.  I have been using it for several months to transcode JS to create a
Querypoint debugger for Chrome. I plan to use it to improve developer support
for using Traceur and to prototype debugger features for ES.next. Robert Hundt
wants to use it to experiment with script compression algorithms for load
performance. It could be used to improve the debugger support for CoffeeScript
and other languages that compile to JS.  It extends the chrome.devtools
extension system in a powerful new direction.

There are of course real costs in support debugger features.  However the risks
here are not that big and the burden of maintenance will anyway fall on those of
us working on the debugger features.

If you want me to do some kind of formal procedure, please let me know what to
do.

[marja, 2013-06-05 09:37:34]

(I'll review the ScopedPersistent usage once you've reached a consensus about
the feature with abarth@.)

[abarth-chromium, 2013-06-07 20:13:45]

https://codereview.chromium.org/13575004/diff/37001/Source/bindings/v8/Script...
File Source/bindings/v8/ScriptController.cpp (right):

https://codereview.chromium.org/13575004/diff/37001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptController.cpp:736: v8::Local<v8::Context> context =
v8::Context::New(m_isolate);
This CL is not lgtm.  Please do not create ad-hoc v8::Contexts.  That breaks the
rest of the bindings.

[johnjbarton, 2013-06-17 19:47:11]

On 2013/06/07 20:13:45, abarth wrote:
>
https://codereview.chromium.org/13575004/diff/37001/Source/bindings/v8/Script...
> File Source/bindings/v8/ScriptController.cpp (right):
> 
>
https://codereview.chromium.org/13575004/diff/37001/Source/bindings/v8/Script...
> Source/bindings/v8/ScriptController.cpp:736: v8::Local<v8::Context> context =
> v8::Context::New(m_isolate);
> This CL is not lgtm.  Please do not create ad-hoc v8::Contexts.  That breaks
the
> rest of the bindings.

Adam, please take a look at the new version. It uses the isolated world system
to create and call the preprocessor function.

One conceptually unsatisfactory aspect is our (lack-of) API for allocating
world-ids. Blink allows callers to assign the world-ids:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex...
Internal uses of world-ids must pick values that hopefully will not collide with
each other or with eg chromium.  I picked 10001, allowing Chromium to use up
10,000 worlds before we collide. See also:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex...

As a practical matter this is fine. But we could move the allocation of
world-ids into Blink (using eg the same algorithm used in chromium).

[johnjbarton, 2013-06-27 16:38:02]

Documentation of the ScriptPreprocessor feature use, use-cases, and
implementation:

https://code.google.com/p/chromium/wiki/ScriptPreprocessor

Feedback welcome.

[abarth-chromium, 2013-06-27 22:33:04]

On 2013/06/27 16:38:02, johnjbarton wrote:
> Feedback welcome.

"The preprocessor runs in an isolated world similar to the environment of Chrome
content scripts. The window object of the page is available but developers
should be cautious about operating on the DOM in the preprocessor."

These two sentences don't make sense together.  If the script is running in an
isolated world, then the window object of the page cannot be available.  We're
close to adding a RELEASE_ASSERT to enforce that once we finish fixing any
lingering bugs that leak objects between worlds.

"In addition, do not call eval() or new Function() in the preprocessor."

Is that enforced?

[johnjbarton, 2013-06-27 23:02:00]

On 2013/06/27 22:33:04, abarth wrote:
> On 2013/06/27 16:38:02, johnjbarton wrote:
> > Feedback welcome.
> 
> "The preprocessor runs in an isolated world similar to the environment of
Chrome
> content scripts. The window object of the page is available but developers
> should be cautious about operating on the DOM in the preprocessor."
> 
> These two sentences don't make sense together.  If the script is running in an
> isolated world, then the window object of the page cannot be available.  We're
> close to adding a RELEASE_ASSERT to enforce that once we finish fixing any
> lingering bugs that leak objects between worlds.

Thanks, I've corrected the text. I was thinking "a window object attached to the
page" when I was writing, but my edit aimed to be even more explicit. The short
version is that this is just like content-scripts.

> 
> "In addition, do not call eval() or new Function() in the preprocessor."
> 
> Is that enforced?

Sorry, I forgot that I covered this case in the patch as well.  There is no such
limitation. The preprocessEval() code checks
ScriptController::isScriptPreprocessing() to avoid this case. I've updated the
document.

[johnjbarton, 2013-07-08 16:38:07]

Adam,

As far as I am aware, I've addressed the major issues here. What else can I
improve in this patch? We still need to hear from Marja and Pavel but I believe
they are waiting for your review.

[abarth-chromium, 2013-07-08 23:57:08]

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
File Source/bindings/v8/PageScriptDebugServer.cpp (right):

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/PageScriptDebugServer.cpp:70: if (frame)
You can merge these two lines.

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/PageScriptDebugServer.cpp:71: return
frame->page()->mainFrame()->script();
What if frame->page() is zero?

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
File Source/bindings/v8/ScriptController.cpp (right):

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:743: : m_controller(controller),
m_isolate(controller->m_isolate)
The correct style is to use separate lines for each initializer:

    : m_controller(controller)
    , m_isolate(controller->m_isolate)

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:745: v8::HandleScope scope(m_isolate);
Is it safe to hold the isolate across call stacks?  Generally, we only hold onto
the isolate within a given call stack.

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:755: ScriptValue preprocessorFunction =
scriptResults[0];
What if scriptResults is empty ?

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:761: return;
I don't understand what you're trying to do there.  Why not just log to the
console instead of creating this fake exception?

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:771: return sourceCode;
You can do this before creating the handleScope

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:777: return sourceCode;
When does this happy?  How can m_preprocessorFunction not be empty but
isolatedWorldShell not be initialized?

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:787: m_isPreprocessing = true;
Please use TemporaryChange to make sure we handle re-entrancy and early returns
correctly.

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:789:
V8ScriptRunner::callAsFunction(m_preprocessorFunction.newLocal(m_isolate),
context->Global(), 2, argv);
2 -> WTF_ARRAY_LENGTH

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:810: }
The compiler will generate this function for you.  You don't need to write it
yourself.

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:814: ScopedPersistent<v8::Function>
m_preprocessorFunction;
Does this leak memory?

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:834: m_preprocessorSource =
preprocessorBody;
Why not store m_preprocessorSource in m_scriptPreprocessor?  We're going to run
a script soon anyway.  We're unlikely to win by creating the script preprocessor
lazily.

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:848: return
m_scriptPreprocessor->preprocessSourceCode(scriptSource, scriptName);
Don't we need to clear the m_scriptPreprocessor on navigation?  The way you've
got things set up currently, m_preprocessorFunction is associated with the
Document that was in the Frame when it was created.  That means retaining
m_preprocessorFunction will retain that Document and its entire DOM for the
lifetime of the Frame.

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
File Source/bindings/v8/ScriptController.h (right):

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptController.h:68: ScriptPreprocessorIsolatedWorldId =
10001,
Why is it safe to steal this world ID?  It's valid for the embedder to use this
world API itself, which would cause a collision.

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptController.h:164: // Empty string disables
preprocessing.
Rather than using in-band signaling, I'd recommend a separate
clearScriptPreprocessor() function.

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptController.h:169: bool isScriptPreprocessing();
isPreprocessingScript

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
File Source/bindings/v8/ScriptDebugServer.cpp (right):

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptDebugServer.cpp:318: // Extract the controller before
we enter the debugContext.
This comment doesn't add anything to the code and should be removed.

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptDebugServer.cpp:333: String typeInfo =
toWebCoreStringWithUndefinedOrNullCheck(callDebuggerMethod("getScriptCompilationTypeInfo",
1, argvEventData));
1 -> WTF_ARRAY_LENGTH(argv)

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/core/inspec...
File Source/core/inspector/PageDebuggerAgent.cpp (right):

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/core/inspec...
Source/core/inspector/PageDebuggerAgent.cpp:141:
frame->script()->setScriptPreprocessor(m_pageAgent->scriptPreprocessor());
Ah, we do clear it each time the Frame navigates.  That's good, but it should be
done by the ScriptController.  What if the user closes the dev tools (or
PageDebuggerAgent shuts down some other way)?

[abarth-chromium, 2013-07-08 23:57:57]

The biggest problem is that you've co-opted a random world ID.  The rest of the
approach looks fine.  There are some minor details to polish, but they shouldn't
be too bad.

[abarth-chromium, 2013-07-09 17:34:37]

One way to solve the co-opted world ID problem is to change the API to
RELEASE_ASSERT that the high bit of the world ID isn't set.  We can then reserve
all world IDs with the high bit set for internal use.  We should check with the
extensions folks to make sure they're ok with that change.

[abarth-chromium, 2013-07-09 17:43:56]

On 2013/07/09 17:34:37, abarth wrote:
> One way to solve the co-opted world ID problem is to change the API to
> RELEASE_ASSERT that the high bit of the world ID isn't set.  We can then
reserve
> all world IDs with the high bit set for internal use.  We should check with
the
> extensions folks to make sure they're ok with that change.

Actually, I think world IDs are signed and we already use the high bit
internally for some craziness.  Maybe we need to steal another bit from the API.

[johnjbarton, 2013-07-09 22:44:54]

Adam, please take another look, esp. at the ScriptPreprocessor in
ScriptController.cpp.

I think I understand your suggestion on 
https://code.google.com/p/chromium/issues/detail?id=258504
and I will create a patch for that (It will require a change here for the
constant).

If you think we are close enough I would like to ask Marja to look at pointer
types and Pavel to look at the ScriptDebugServer and tests.

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
File Source/bindings/v8/PageScriptDebugServer.cpp (right):

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/PageScriptDebugServer.cpp:70: if (frame)
On 2013/07/08 23:57:08, abarth wrote:
> You can merge these two lines.

I tried that but the presumbit failed with:
More than one command on the same line in if  [whitespace/parens] [4]
(note the conditional now has two sub expressions)

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/PageScriptDebugServer.cpp:71: return
frame->page()->mainFrame()->script();
On 2013/07/08 23:57:08, abarth wrote:
> What if frame->page() is zero?

Done.

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
File Source/bindings/v8/ScriptController.cpp (right):

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:743: : m_controller(controller),
m_isolate(controller->m_isolate)
On 2013/07/08 23:57:08, abarth wrote:
> The correct style is to use separate lines for each initializer:
> 
>     : m_controller(controller)
>     , m_isolate(controller->m_isolate)

Done.

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:745: v8::HandleScope scope(m_isolate);
On 2013/07/08 23:57:08, abarth wrote:
> Is it safe to hold the isolate across call stacks?  Generally, we only hold
onto
> the isolate within a given call stack.

ScriptController holds an isolate so I guess it is ok. Nevertheless I think this
patch is better without having the script preprocessor hold an isolate. That way
the identity of the isolate we use is clearer.  I removed the member m_isolate.

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:755: ScriptValue preprocessorFunction =
scriptResults[0];
On 2013/07/08 23:57:08, abarth wrote:
> What if scriptResults is empty ?

executeScriptInIsolatedWorld maps sources onto scriptResults: the arrays lengths
must match. I added an ASSERT to avoid a deref error if later changes to the
function change the API.

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:761: return;
On 2013/07/08 23:57:08, abarth wrote:
> I don't understand what you're trying to do there.  Why not just log to the
> console instead of creating this fake exception?

Done.

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:771: return sourceCode;
On 2013/07/08 23:57:08, abarth wrote:
> You can do this before creating the handleScope

Done, pls recheck the new location.

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:777: return sourceCode;
On 2013/07/08 23:57:08, abarth wrote:
> When does this happy?  How can m_preprocessorFunction not be empty but
> isolatedWorldShell not be initialized?

Done.

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:787: m_isPreprocessing = true;
On 2013/07/08 23:57:08, abarth wrote:
> Please use TemporaryChange to make sure we handle re-entrancy and early
returns
> correctly.

Done.

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:789:
V8ScriptRunner::callAsFunction(m_preprocessorFunction.newLocal(m_isolate),
context->Global(), 2, argv);
On 2013/07/08 23:57:08, abarth wrote:
> 2 -> WTF_ARRAY_LENGTH

Done.

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:814: ScopedPersistent<v8::Function>
m_preprocessorFunction;
On 2013/07/08 23:57:08, abarth wrote:
> Does this leak memory?

Destructing the ScriptPreprocessor destructs the m_preprocessorFunction per
ScopedPersistent.h. The ScriptPreprocessor is OwnPtr and it is clear()-ed on
reload.  Therefore I believe this memory cannot leak.

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:834: m_preprocessorSource =
preprocessorBody;
On 2013/07/08 23:57:08, abarth wrote:
> Why not store m_preprocessorSource in m_scriptPreprocessor?  We're going to
run
> a script soon anyway.  We're unlikely to win by creating the script
preprocessor
> lazily.

At this point in the page setup the debugger has not yet injected scripts into
the page. Writing on the page console (because of an error in the preprocessor)
at this point causes console injection. In earlier versions of this patch I hit
infinite recursion because of interaction between injection and preprocessing. I
don't believe the current version would have this problem. However since this is
very nasty problem to unravel I think we're better off maintaining the code to
support the delayed evaluation.

I changed the comment to try to clarify the motivation.

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:848: return
m_scriptPreprocessor->preprocessSourceCode(scriptSource, scriptName);
On 2013/07/08 23:57:08, abarth wrote:
> Don't we need to clear the m_scriptPreprocessor on navigation?  The way you've
> got things set up currently, m_preprocessorFunction is associated with the
> Document that was in the Frame when it was created.  That means retaining
> m_preprocessorFunction will retain that Document and its entire DOM for the
> lifetime of the Frame.

We clear the m_scriptPreprocessor whenever
PageDebuggerAgent::didClearWindowObjectInWorld() is called. Navigation will
cause this function to be called. Thus we do clear on navigation.

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
File Source/bindings/v8/ScriptController.h (right):

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptController.h:68: ScriptPreprocessorIsolatedWorldId =
10001,
On 2013/07/08 23:57:08, abarth wrote:
> Why is it safe to steal this world ID?  It's valid for the embedder to use
this
> world API itself, which would cause a collision.

It is not absolutely safe, only relatively safe given that the only known
embedder using world ids starts at 1 and increments by 1 in a session unlikely
to include thousands of worlds.

Let's discuss how to solve this on:
https://code.google.com/p/chromium/issues/detail?id=258504

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptController.h:164: // Empty string disables
preprocessing.
On 2013/07/08 23:57:08, abarth wrote:
> Rather than using in-band signaling, I'd recommend a separate
> clearScriptPreprocessor() function.

Done.

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptController.h:169: bool isScriptPreprocessing();
On 2013/07/08 23:57:08, abarth wrote:
> isPreprocessingScript

Done.

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
File Source/bindings/v8/ScriptDebugServer.cpp (right):

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptDebugServer.cpp:318: // Extract the controller before
we enter the debugContext.
On 2013/07/08 23:57:08, abarth wrote:
> This comment doesn't add anything to the code and should be removed.

Done.

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/bindings/v8...
Source/bindings/v8/ScriptDebugServer.cpp:333: String typeInfo =
toWebCoreStringWithUndefinedOrNullCheck(callDebuggerMethod("getScriptCompilationTypeInfo",
1, argvEventData));
On 2013/07/08 23:57:08, abarth wrote:
> 1 -> WTF_ARRAY_LENGTH(argv)

Done.

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/core/inspec...
File Source/core/inspector/PageDebuggerAgent.cpp (right):

https://chromiumcodereview.appspot.com/13575004/diff/70001/Source/core/inspec...
Source/core/inspector/PageDebuggerAgent.cpp:141:
frame->script()->setScriptPreprocessor(m_pageAgent->scriptPreprocessor());
On 2013/07/08 23:57:08, abarth wrote:
> Ah, we do clear it each time the Frame navigates.  That's good, but it should
be
> done by the ScriptController.  What if the user closes the dev tools (or
> PageDebuggerAgent shuts down some other way)?

I'll need to check with Pavel Feldman, but I believe that closing the front-end
has no effect on the dispatch of didClearWindowObjectInWorld() until the next
navigation/reload.

[abarth-chromium, 2013-07-09 23:14:10]

I'm basically happy with this CL except for the isolated wold ID and memory leak
issues.  I would like to review it again before it lands, but other folks should
feel free to review it as well.

https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/bindings/v8...
File Source/bindings/v8/ScriptController.cpp (right):

https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:741: class
ScriptController::ScriptPreprocessor {
Please put this class in a separate file.  We prefer to have one class per file.
 There isn't really any reason for this class to be an inner class of
ScriptController.

https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:756: ASSERT(scriptResults.size() == 1);
Please handle this error instead of just ASSERTing the result.

https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:760: return;
You're already set up to handle an error here.  Why not handle the case where
scriptResult.size() is an unexpected value?

https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:770: RefPtr<DOMWrapperWorld> world =
DOMWrapperWorld::ensureIsolatedWorld(ScriptPreprocessorIsolatedWorldId, 0);
What is the 0 in the second position here?

https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:793:
V8ScriptRunner::callAsFunction(m_preprocessorFunction.newLocal(isolate),
context->Global(), WTF_ARRAY_LENGTH(argv), argv);
You can combine these lines.  There's no line limit here.

https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:838: m_preprocessorSource = "";
m_preprocessorSource = String();

That will get you back to exactly the state you started in (i.e., having a null
string---not an empty string).

https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:847: m_scriptPreprocessor = adoptPtr(new
ScriptPreprocessor(m_preprocessorSource, this,
this->m_frame->page()->console()));
this->m_frame   ->    m_frame

The "this->" isn't needed.

What if page() is zero?

https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/bindings/v8...
File Source/bindings/v8/ScriptController.h (right):

https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/bindings/v8...
Source/bindings/v8/ScriptController.h:68: ScriptPreprocessorIsolatedWorldId =
10001,
Yeah, we mostly need to change this constant to (1 << 30) + 1 and add a
RELEASE_ASSERT to the API.

https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/bindings/v8...
File Source/bindings/v8/ScriptDebugServer.cpp (right):

https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/bindings/v8...
Source/bindings/v8/ScriptDebugServer.cpp:339: String patchedScript =
controller->preprocess(script, scriptName);
|controller| might be pointing to an object that has already been freed at this
point.  Instead of blindly using the point, you need to retrieve it from
eventContext again and check the return value for zero.

https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/bindings/v8...
Source/bindings/v8/ScriptDebugServer.cpp:341: v8::Handle<v8::Value> argv2[] = {
eventData, v8String(patchedScript, debugContext->GetIsolate()) };
argv2   ->   Please use a more descriptive name for these variables.

https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/core/inspec...
File Source/core/inspector/PageDebuggerAgent.cpp (right):

https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/core/inspec...
Source/core/inspector/PageDebuggerAgent.cpp:141:
frame->script()->clearScriptPreprocessor();
Please clear the script preprocessor in ScriptController.  The ScriptController
should have enough knowledge to understand that it's not leaking the world by
holding the script preprocessor.  We shouldn't have to reason able the
PageDebuggerAgent to come to that conclusion.

[johnjbarton, 2013-07-10 23:24:22]

Adam, Please take another look; 2 issues you raised I could not resolve.

https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/bindings/v8...
File Source/bindings/v8/ScriptController.cpp (right):

https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:756: ASSERT(scriptResults.size() == 1);
On 2013/07/09 23:14:10, abarth wrote:
> Please handle this error instead of just ASSERTing the result.

Done.

https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:760: return;
On 2013/07/09 23:14:10, abarth wrote:
> You're already set up to handle an error here.  Why not handle the case where
> scriptResult.size() is an unexpected value?

Done.

https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:771: V8WindowShell* isolatedWorldShell =
m_controller->windowShell(world.get());
This groups the isolatedWorld with other worlds that will use the same V8
extensions. I replaced the 0 with the const value
DOMWrapperWorld::mainWorldExtensionGroup since we do not want any V8 extensions
not also used by the page. I also changed the similar 0 value in the
executeScriptInIsolatedWorld() call.

https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:793:
V8ScriptRunner::callAsFunction(m_preprocessorFunction.newLocal(isolate),
context->Global(), WTF_ARRAY_LENGTH(argv), argv);
On 2013/07/09 23:14:10, abarth wrote:
> You can combine these lines.  There's no line limit here.

Done.

https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:838: m_preprocessorSource = "";
On 2013/07/09 23:14:10, abarth wrote:
> m_preprocessorSource = String();
> 
> That will get you back to exactly the state you started in (i.e., having a
null
> string---not an empty string).

Done.

https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:847: m_scriptPreprocessor = adoptPtr(new
ScriptPreprocessor(m_preprocessorSource, this,
this->m_frame->page()->console()));
On 2013/07/09 23:14:10, abarth wrote:
> this->m_frame   ->    m_frame
> 
> The "this->" isn't needed.

Done

> 
> What if page() is zero?

I added a test for this case.

https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/bindings/v8...
File Source/bindings/v8/ScriptController.h (right):

https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/bindings/v8...
Source/bindings/v8/ScriptController.h:68: ScriptPreprocessorIsolatedWorldId =
10001,
On 2013/07/09 23:14:10, abarth wrote:
> Yeah, we mostly need to change this constant to (1 << 30) + 1 and add a
> RELEASE_ASSERT to the API.

Done.

https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/bindings/v8...
File Source/bindings/v8/ScriptDebugServer.cpp (right):

https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/bindings/v8...
Source/bindings/v8/ScriptDebugServer.cpp:339: String patchedScript =
controller->preprocess(script, scriptName);
On 2013/07/09 23:14:10, abarth wrote:
> |controller| might be pointing to an object that has already been freed at
this
> point.  Instead of blindly using the point, you need to retrieve it from
> eventContext again and check the return value for zero.

We can't compute the ScriptController within the debugContext scope. I believe
that the reason is that V8Binding worldTypeInMainThread() does not use its
argument 'isolate'. Instead it computes the WrapperWorldType from
v8::Context::GetEntered(), which is v8::Debug::GetDebugContext() not the context
for our isolated world.

That is why I earlier had a comment that the controller must be computed before
entering the debugContext scope.

https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/bindings/v8...
Source/bindings/v8/ScriptDebugServer.cpp:341: v8::Handle<v8::Value> argv2[] = {
eventData, v8String(patchedScript, debugContext->GetIsolate()) };
On 2013/07/09 23:14:10, abarth wrote:
> argv2   ->   Please use a more descriptive name for these variables.

Done.

https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/core/inspec...
File Source/core/inspector/PageDebuggerAgent.cpp (right):

https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/core/inspec...
Source/core/inspector/PageDebuggerAgent.cpp:141:
frame->script()->clearScriptPreprocessor();
On 2013/07/09 23:14:10, abarth wrote:
> Please clear the script preprocessor in ScriptController.  The
ScriptController
> should have enough knowledge to understand that it's not leaking the world by
> holding the script preprocessor.  We shouldn't have to reason able the
> PageDebuggerAgent to come to that conclusion.

Sorry I don't understand how to do this.

I assume we would need to have a call into a ScriptController function every
time that didClearWindowObjectInWorld() is called and we would have to know that
such a call always precedes the PageDebuggerAgent call (to avoid clearing valid
preprocessor sources).  Do we have such a function?

[abarth-chromium, 2013-07-10 23:27:02]

On 2013/07/10 23:24:22, johnjbarton wrote:
>
https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/core/inspec...
> Source/core/inspector/PageDebuggerAgent.cpp:141:
> frame->script()->clearScriptPreprocessor();
> On 2013/07/09 23:14:10, abarth wrote:
> > Please clear the script preprocessor in ScriptController.  The
> ScriptController
> > should have enough knowledge to understand that it's not leaking the world
by
> > holding the script preprocessor.  We shouldn't have to reason able the
> > PageDebuggerAgent to come to that conclusion.
> 
> Sorry I don't understand how to do this.
> 
> I assume we would need to have a call into a ScriptController function every
> time that didClearWindowObjectInWorld() is called and we would have to know
that
> such a call always precedes the PageDebuggerAgent call (to avoid clearing
valid
> preprocessor sources).  Do we have such a function?

Does ScriptController::clearWindowShell work?

[johnjbarton, 2013-07-10 23:39:26]

On 2013/07/10 23:27:02, abarth wrote:
> On 2013/07/10 23:24:22, johnjbarton wrote:
> >
>
https://chromiumcodereview.appspot.com/13575004/diff/81001/Source/core/inspec...
> > Source/core/inspector/PageDebuggerAgent.cpp:141:
> > frame->script()->clearScriptPreprocessor();
> > On 2013/07/09 23:14:10, abarth wrote:
> > > Please clear the script preprocessor in ScriptController.  The
> > ScriptController
> > > should have enough knowledge to understand that it's not leaking the world
> by
> > > holding the script preprocessor.  We shouldn't have to reason able the
> > > PageDebuggerAgent to come to that conclusion.
> > 
> > Sorry I don't understand how to do this.
> > 
> > I assume we would need to have a call into a ScriptController function every
> > time that didClearWindowObjectInWorld() is called and we would have to know
> that
> > such a call always precedes the PageDebuggerAgent call (to avoid clearing
> valid
> > preprocessor sources).  Do we have such a function?
> 
> Does ScriptController::clearWindowShell work?

Done

[abarth-chromium, 2013-07-10 23:53:50]

Great!  Would still like other folks to look at the CL or should I do the final
review?

[haraken, 2013-07-11 00:00:19]

I'll take a look in an hour.

[haraken, 2013-07-11 01:13:59]

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
File Source/bindings/v8/PageScriptDebugServer.cpp (right):

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
Source/bindings/v8/PageScriptDebugServer.cpp:71: return
frame->page()->mainFrame()->script();

I'm curious about how this is different from frame->script().

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
File Source/bindings/v8/ScriptController.cpp (right):

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:670: String processedString =
m_frame->script()->preprocess(sourceCode.source(), sourceURL);

What's the difference between preprocess() and m_frame->script()->preprocess() ?

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:742: String
ScriptController::preprocess(const String& scriptSource, const String&
scriptName)

Would you add a comment here about the exact condition under which the
preprocessing is conducted?

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:757: }

I'm not happy with having a lot of proxy methods to ScriptPreprocessor. Also the
methods are not well organized. (Do we really need all of hasScriptPreprocess,
isPreprocessing, m_preprocessor.isEmpty, m_scriptPreprocessor etc ?) It's very
confusing when we should check what. Please clean up the methods.

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
File Source/bindings/v8/ScriptController.h (right):

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
Source/bindings/v8/ScriptController.h:68: enum IsolatedWorldConstants {

Shall we move this to ScriptPreprocessor.h?

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
File Source/bindings/v8/ScriptDebugServer.cpp (right):

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
Source/bindings/v8/ScriptDebugServer.cpp:448: v8::Handle<v8::Function>
getAfterCompileScript =
v8::Local<v8::Function>::Cast(m_debuggerScript.get()->Get(v8::String::NewSymbol("getAfterCompileScript")));

ScopedPersistent::get() returns an unsafe handle and is going to be deprecated
(as commented in ScopedPersistent). Please create a local handle and use it like
this:

  v8::Handle<v8::Object> debuggerScript = m_debuggerScript.newLocal(m_isolate);
  debuggerScript->Get(...);

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
Source/bindings/v8/ScriptDebugServer.cpp:450: v8::Handle<v8::Value> value =
V8ScriptRunner::callInternalFunction(getAfterCompileScript,
m_debuggerScript.get(), WTF_ARRAY_LENGTH(argv), argv, m_isolate);

Ditto.

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
Source/bindings/v8/ScriptDebugServer.cpp:465: v8::Handle<v8::Function>
getBreakpointNumbersFunction =
v8::Local<v8::Function>::Cast(m_debuggerScript.get()->Get(v8::String::NewSymbol("getBreakpointNumbers")));

Ditto.

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
Source/bindings/v8/ScriptDebugServer.cpp:467: v8::Handle<v8::Value>
hitBreakpoints =
V8ScriptRunner::callInternalFunction(getBreakpointNumbersFunction,
m_debuggerScript.get(), WTF_ARRAY_LENGTH(argv), argv, m_isolate);

Ditto.

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
File Source/bindings/v8/ScriptPreprocessor.cpp (right):

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
Source/bindings/v8/ScriptPreprocessor.cpp:73: v8::Isolate* isolate =
context->GetIsolate();

Instead of looking up the Isolate every time, you can pass an Isolate when
constructing ScriptPreprocessor, store the Isolate to
ScriptPreprocessor::m_isolate, and use it afterward.

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
Source/bindings/v8/ScriptPreprocessor.cpp:77: v8::Local<v8::Context>
ScriptPreprocessor::isolatedWorldContext()

It looks wasteful to call isolatedWorldContext() and look up the context every
time ScriptPreprocessor needs a context. Conceptually, I think
ScriptPreprocessor should own the context. You can add
ScriptPreprocessor::m_context, initialize it in the constructor of
ScriptPreprocessor, and use it afterward.

[johnjbarton, 2013-07-11 19:14:42]

Haraken, I've made changes to address your comments, please take another look.

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
File Source/bindings/v8/PageScriptDebugServer.cpp (right):

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
Source/bindings/v8/PageScriptDebugServer.cpp:71: return
frame->page()->mainFrame()->script();
On 2013/07/11 01:13:59, haraken wrote:
> 
> I'm curious about how this is different from frame->script().

Done.

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
File Source/bindings/v8/ScriptController.cpp (right):

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:670: String processedString =
m_frame->script()->preprocess(sourceCode.source(), sourceURL);
On 2013/07/11 01:13:59, haraken wrote:
> 
> What's the difference between preprocess() and m_frame->script()->preprocess()
?

Done.

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:742: String
ScriptController::preprocess(const String& scriptSource, const String&
scriptName)
On 2013/07/11 01:13:59, haraken wrote:
> 
> Would you add a comment here about the exact condition under which the
> preprocessing is conducted?

Done. (The comment was also in the .h file).

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
Source/bindings/v8/ScriptController.cpp:757: }
On 2013/07/11 01:13:59, haraken wrote:
> 
> I'm not happy with having a lot of proxy methods to ScriptPreprocessor. Also
the
> methods are not well organized. (Do we really need all of hasScriptPreprocess,
> isPreprocessing, m_preprocessor.isEmpty, m_scriptPreprocessor etc ?) It's very
> confusing when we should check what. Please clean up the methods.

Two objectives lead to proxy methods:
  1) encapsulate the ScriptPreprocessor to avoid distributing the logic for its
lazy creation (lazy creation motivated by avoiding entangling preprocessing and
console script injection).
  2) conditional access to the ScriptPreprocessor by ScriptDebugServer for
eval() processing. 

I think #1 is important. 

To avoid #2 we would have to call into the debugger-specific functions of
ScriptDebugServer from ScriptPreprocessor. I implemented this version for your
evaluation.

Of course my logic could be flawed or there may be another solution.

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
File Source/bindings/v8/ScriptController.h (right):

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
Source/bindings/v8/ScriptController.h:68: enum IsolatedWorldConstants {
On 2013/07/11 01:13:59, haraken wrote:
> 
> Shall we move this to ScriptPreprocessor.h?

The most logical place would be DOMWrapperWorld.h. I moved the enum to that
file.

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
File Source/bindings/v8/ScriptDebugServer.cpp (right):

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
Source/bindings/v8/ScriptDebugServer.cpp:448: v8::Handle<v8::Function>
getAfterCompileScript =
v8::Local<v8::Function>::Cast(m_debuggerScript.get()->Get(v8::String::NewSymbol("getAfterCompileScript")));
On 2013/07/11 01:13:59, haraken wrote:
> 
> ScopedPersistent::get() returns an unsafe handle and is going to be deprecated
> (as commented in ScopedPersistent). Please create a local handle and use it
like
> this:
> 
>   v8::Handle<v8::Object> debuggerScript =
m_debuggerScript.newLocal(m_isolate);
>   debuggerScript->Get(...);

Done.

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
Source/bindings/v8/ScriptDebugServer.cpp:450: v8::Handle<v8::Value> value =
V8ScriptRunner::callInternalFunction(getAfterCompileScript,
m_debuggerScript.get(), WTF_ARRAY_LENGTH(argv), argv, m_isolate);
On 2013/07/11 01:13:59, haraken wrote:
> 
> Ditto.

Done.

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
Source/bindings/v8/ScriptDebugServer.cpp:465: v8::Handle<v8::Function>
getBreakpointNumbersFunction =
v8::Local<v8::Function>::Cast(m_debuggerScript.get()->Get(v8::String::NewSymbol("getBreakpointNumbers")));
On 2013/07/11 01:13:59, haraken wrote:
> 
> Ditto.

Done.

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
Source/bindings/v8/ScriptDebugServer.cpp:467: v8::Handle<v8::Value>
hitBreakpoints =
V8ScriptRunner::callInternalFunction(getBreakpointNumbersFunction,
m_debuggerScript.get(), WTF_ARRAY_LENGTH(argv), argv, m_isolate);
On 2013/07/11 01:13:59, haraken wrote:
> 
> Ditto.

Done.

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
File Source/bindings/v8/ScriptPreprocessor.cpp (right):

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
Source/bindings/v8/ScriptPreprocessor.cpp:73: v8::Isolate* isolate =
context->GetIsolate();
On 2013/07/11 01:13:59, haraken wrote:
> 
> Instead of looking up the Isolate every time, you can pass an Isolate when
> constructing ScriptPreprocessor, store the Isolate to
> ScriptPreprocessor::m_isolate, and use it afterward.

Done.

https://chromiumcodereview.appspot.com/13575004/diff/97001/Source/bindings/v8...
Source/bindings/v8/ScriptPreprocessor.cpp:77: v8::Local<v8::Context>
ScriptPreprocessor::isolatedWorldContext()
On 2013/07/11 01:13:59, haraken wrote:
> 
> It looks wasteful to call isolatedWorldContext() and look up the context every
> time ScriptPreprocessor needs a context. Conceptually, I think
> ScriptPreprocessor should own the context. You can add
> ScriptPreprocessor::m_context, initialize it in the constructor of
> ScriptPreprocessor, and use it afterward.

Done.

[haraken, 2013-07-12 00:13:18]

LGTM. Please wait for abarth's approval.

https://chromiumcodereview.appspot.com/13575004/diff/102001/Source/bindings/v...
File Source/bindings/v8/ScriptController.cpp (right):

https://chromiumcodereview.appspot.com/13575004/diff/102001/Source/bindings/v...
Source/bindings/v8/ScriptController.cpp:733: String
ScriptController::preprocess(const String& scriptSource, const String&
scriptName)

Just to confirm: preprocess() does nothing when a normal web page is loaded or
event listeners are fired, and thus there is no overhead. Right?

[abarth-chromium, 2013-07-12 00:25:15]

LGTM

Thanks for iterating on this CL.  I'm sorry if the process was painful at times.
 I'm happy with the result we ended up with.

https://chromiumcodereview.appspot.com/13575004/diff/102001/Source/bindings/v...
File Source/bindings/v8/ScriptController.cpp (right):

https://chromiumcodereview.appspot.com/13575004/diff/102001/Source/bindings/v...
Source/bindings/v8/ScriptController.cpp:733: String
ScriptController::preprocess(const String& scriptSource, const String&
scriptName)
On 2013/07/12 00:13:19, haraken wrote:
> Just to confirm: preprocess() does nothing when a normal web page is loaded or
> event listeners are fired, and thus there is no overhead. Right?

Yeah, m_preprocessorSource will be null.

https://chromiumcodereview.appspot.com/13575004/diff/102001/Source/bindings/v...
File Source/bindings/v8/ScriptPreprocessor.cpp (right):

https://chromiumcodereview.appspot.com/13575004/diff/102001/Source/bindings/v...
Source/bindings/v8/ScriptPreprocessor.cpp:59:
controller->executeScriptInIsolatedWorld(ScriptPreprocessorIsolatedWorldId,
sources, DOMWrapperWorld::mainWorldExtensionGroup, &scriptResults);
Should this be "executeInternalScriptInIsolatedWorld?  It seems like the
preprocessorScript should count as an internal script.

https://chromiumcodereview.appspot.com/13575004/diff/102001/Source/core/inspe...
File Source/core/inspector/PageDebuggerAgent.cpp (right):

https://chromiumcodereview.appspot.com/13575004/diff/102001/Source/core/inspe...
Source/core/inspector/PageDebuggerAgent.cpp:142: if
(!m_pageAgent->scriptPreprocessor().isEmpty())
Is this branch correct?  I would expect that if the pageAgent's
scriptPreprocessor became empty, we'd propagate that to the ScriptController at
this point.

[johnjbarton, 2013-07-12 17:56:49]

Adam or Haraken:

I made three additional changes since your lgtm:

1) Restored a line I deleted in ScriptDebugServer.cpp
   v8::HandleScope scope(m_isolate);
2) Added inclusion #ifdef guards to ScriptPreprocessor.h,
3) Added comment to clarify PageDebuggerAgent.

Please check, esp. item #1.

https://chromiumcodereview.appspot.com/13575004/diff/102001/Source/bindings/v...
File Source/bindings/v8/ScriptDebugServer.cpp (left):

https://chromiumcodereview.appspot.com/13575004/diff/102001/Source/bindings/v...
Source/bindings/v8/ScriptDebugServer.cpp:475: v8::HandleScope scope(m_isolate);
I believe that omitting this line is an error. I have restored it in the next
patch. Please check me.

https://chromiumcodereview.appspot.com/13575004/diff/102001/Source/bindings/v...
File Source/bindings/v8/ScriptPreprocessor.cpp (right):

https://chromiumcodereview.appspot.com/13575004/diff/102001/Source/bindings/v...
Source/bindings/v8/ScriptPreprocessor.cpp:59:
controller->executeScriptInIsolatedWorld(ScriptPreprocessorIsolatedWorldId,
sources, DOMWrapperWorld::mainWorldExtensionGroup, &scriptResults);
On 2013/07/12 00:25:16, abarth wrote:
> Should this be "executeInternalScriptInIsolatedWorld?  It seems like the
> preprocessorScript should count as an internal script.

I'm unsure what you mean here. I was not able to find
"executeInternalScriptInIsolatedWorld", though I recall such a function in the
past. 

V8ScriptRunner has compileAndRunInternalScript() and callInternalFunction().
These functions, when compared to the similar functions without 'Internal', skip
the memory and stack-limitation checks. Since the preprocessor is developed
outside of Chrome and because preprocessor developers would benefit from these
checks, I believe the current code is the better choice.

https://chromiumcodereview.appspot.com/13575004/diff/102001/Source/core/inspe...
File Source/core/inspector/PageDebuggerAgent.cpp (right):

https://chromiumcodereview.appspot.com/13575004/diff/102001/Source/core/inspe...
Source/core/inspector/PageDebuggerAgent.cpp:142: if
(!m_pageAgent->scriptPreprocessor().isEmpty())
On 2013/07/12 00:25:16, abarth wrote:
> Is this branch correct?  I would expect that if the pageAgent's
> scriptPreprocessor became empty, we'd propagate that to the ScriptController
at
> this point.

Yes test is correct: if the PageAgent has a scriptPreprocessor, load it in to
the controller.

In earlier versions we did pass the preprocessor source to the controller and we
used its empty status to clear the preprocessor object. You preferred at that
time to avoid this 'inline' signal. You also wanted the ScriptController to
exert direct control over the clear operation, and not delegate that
responsibility to PageDebuggerAgent. 

I added a comment to this code:
 // ScriptController::clearWindowShell() clears its ScriptPreprocessor before
didClearWindowObjectInWorld is dispatched.

[abarth-chromium, 2013-07-12 18:00:26]

Ok.  I'm not sure I understand fully, but it seems fine either way.

[johnjbarton, 2013-07-12 18:29:06]

On 2013/07/12 00:25:15, abarth wrote:
> Thanks for iterating on this CL.  I'm sorry if the process was painful at
times.
>  I'm happy with the result we ended up with.

Thanks for everyone's detailed comments and help!

[haraken, 2013-07-13 00:37:04]

> 1) Restored a line I deleted in ScriptDebugServer.cpp
>    v8::HandleScope scope(m_isolate);
> 2) Added inclusion #ifdef guards to ScriptPreprocessor.h,
> 3) Added comment to clarify PageDebuggerAgent.

This looks fine. Thanks!

https://chromiumcodereview.appspot.com/13575004/diff/102001/Source/bindings/v...
File Source/bindings/v8/ScriptPreprocessor.cpp (right):

https://chromiumcodereview.appspot.com/13575004/diff/102001/Source/bindings/v...
Source/bindings/v8/ScriptPreprocessor.cpp:59:
controller->executeScriptInIsolatedWorld(ScriptPreprocessorIsolatedWorldId,
sources, DOMWrapperWorld::mainWorldExtensionGroup, &scriptResults);

I think we should implement executeInternalScriptInIsolatedWorld() and use it
here, since the script we're going to execute is an internal script (i.e. not a
user script from a web page).

> without 'Internal', skip the memory and stack-limitation checks

More importantly, the difference between internal scripts and user scripts is
that user scripts have to clean up a couple of things (clear IDB transactions,
flush mutation observers etc) when the execution is finished. We should do the
clean-up for user scripts but shouldn't do the clean-up for internal scripts.

See here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[johnjbarton, 2013-07-16 17:22:12]

On 2013/07/13 00:37:04, haraken wrote:
>
https://chromiumcodereview.appspot.com/13575004/diff/102001/Source/bindings/v...
> File Source/bindings/v8/ScriptPreprocessor.cpp (right):
> 
>
https://chromiumcodereview.appspot.com/13575004/diff/102001/Source/bindings/v...
> Source/bindings/v8/ScriptPreprocessor.cpp:59:
> controller->executeScriptInIsolatedWorld(ScriptPreprocessorIsolatedWorldId,
> sources, DOMWrapperWorld::mainWorldExtensionGroup, &scriptResults);
> 
> I think we should implement executeInternalScriptInIsolatedWorld() and use it
> here, since the script we're going to execute is an internal script (i.e. not
a
> user script from a web page).
> 
> > without 'Internal', skip the memory and stack-limitation checks
> 
> More importantly, the difference between internal scripts and user scripts is
> that user scripts have to clean up a couple of things (clear IDB transactions,
> flush mutation observers etc) when the execution is finished. We should do the
> clean-up for user scripts but shouldn't do the clean-up for internal scripts.
> 
> See here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I opened 
https://code.google.com/p/chromium/issues/detail?id=260490
to decide how to make this improvement.

I'm closing this issue because it cannot land, the base URL set at creation time
is from pre-blink WebKit repo.