Apply script preprocessor to Web page scripts only.

Source/bindings/v8/ScriptController.cpp

Index: Source/bindings/v8/ScriptController.cpp
diff --git a/Source/bindings/v8/ScriptController.cpp b/Source/bindings/v8/ScriptController.cpp
index d0085c4491eaa9b9bec14c81ea736845f5adc1c2..a3fdcde2a26bb269fb951da0c551d77bbf29ff07 100644
--- a/Source/bindings/v8/ScriptController.cpp
+++ b/Source/bindings/v8/ScriptController.cpp
@@ -668,7 +668,7 @@ bool ScriptController::executeScriptIfJavaScriptURL(const KURL& url)
 
     if (!locationChangeBefore && m_frame->navigationScheduler()->locationChangePending())
         return true;
-        
+
     // DocumentWriter::replaceDocument can cause the DocumentLoader to get deref'ed and possible destroyed,
     // so protect it with a RefPtr.
     if (RefPtr<DocumentLoader> loader = m_frame->document()->loader())
@@ -687,9 +687,12 @@ ScriptValue ScriptController::executeScriptInMainWorld(const ScriptSourceCode& s
     if (v8Context.IsEmpty())
         return ScriptValue();
 
+    String processedString = m_frame->script()->preprocess(sourceCode.source(), sourceURL);
+    ScriptSourceCode processedSourceCode(processedString, sourceCode.url(), sourceCode.startPosition());
+
     v8::Context::Scope scope(v8Context);
     RefPtr<Frame> protect(m_frame);
-    v8::Local<v8::Value> object = compileAndRunScript(sourceCode);
+    v8::Local<v8::Value> object = compileAndRunScript(processedSourceCode);
 
     m_sourceURL = savedSourceURL;
 
@@ -733,4 +736,117 @@ void ScriptController::executeScriptInIsolatedWorld(int worldID, const Vector<Sc
     }
 }
 
+class ScriptController::ScriptPreprocessor {
+    WTF_MAKE_NONCOPYABLE(ScriptPreprocessor);
+public:
+    ScriptPreprocessor(const String& preprocessorScript, ScriptController* controller)
+        : m_controller(controller), m_isolate(controller->m_isolate)

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

The correct style is to use separate lines for each initializer:

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

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

Done.

+    {
+        v8::HandleScope scope(m_isolate);

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

Is it safe to hold the isolate across call stacks?  Generally, we only hold onto
the isolate within a given call stack.

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

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.

+
+        v8::TryCatch tryCatch;
+        tryCatch.SetVerbose(true);
+        ScriptSourceCode preprocessor(preprocessorScript);
+        Vector<ScriptSourceCode> sources;
+        sources.append(preprocessor);
+        Vector<ScriptValue> scriptResults;
+        controller->executeScriptInIsolatedWorld(ScriptPreprocessorIsolatedWorldId, sources, 0, &scriptResults);
+
+        ScriptValue preprocessorFunction = scriptResults[0];

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

What if scriptResults is empty ?

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

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.

+        if (!preprocessorFunction.isFunction()) {
+            v8::Context::Scope contextScope(m_controller->mainWorldContext());
+            String notAFunction = String("The preprocessor must compile to a function.");
+            String preprocessorName = String("reloadPreprocessorOptionSource.js");
+            getScriptExecutionContext()->reportException(notAFunction, 1, preprocessorName, RefPtr<ScriptCallStack>());
+            return;

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

I don't understand what you're trying to do there.  Why not just log to the
console instead of creating this fake exception?

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

Done.

+        }
+
+        m_preprocessorFunction.set(m_isolate, v8::Handle<v8::Function>::Cast(preprocessorFunction.v8Value()));
+    }
+
+    String preprocessSourceCode(const String& sourceCode, const String& sourceName)
+    {
+        v8::HandleScope handleScope(m_isolate);
+        if (m_preprocessorFunction.isEmpty())
+            return sourceCode;

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

You can do this before creating the handleScope

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

Done, pls recheck the new location.

+
+        RefPtr<DOMWrapperWorld> world = DOMWrapperWorld::ensureIsolatedWorld(ScriptPreprocessorIsolatedWorldId, 0);
+        V8WindowShell* isolatedWorldShell = m_controller->windowShell(world.get());
+
+        if (!isolatedWorldShell->isContextInitialized())
+            return sourceCode;

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

When does this happy?  How can m_preprocessorFunction not be empty but
isolatedWorldShell not be initialized?

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

Done.

+
+        v8::Local<v8::Context> context = isolatedWorldShell->context();
+        v8::Context::Scope contextScope(context);
+        v8::Handle<v8::String> sourceCodeString = v8String(sourceCode, m_isolate);
+        v8::Handle<v8::String> sourceNameString = v8String(sourceName, m_isolate);
+        v8::Handle<v8::Value> argv[] = { sourceCodeString, sourceNameString };
+
+        v8::TryCatch tryCatch;
+        tryCatch.SetVerbose(true);
+        m_isPreprocessing = true;

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

Please use TemporaryChange to make sure we handle re-entrancy and early returns
correctly.

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

Done.

+        v8::Handle<v8::Value> resultValue =
+            V8ScriptRunner::callAsFunction(m_preprocessorFunction.newLocal(m_isolate), context->Global(), 2, argv);

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

2 -> WTF_ARRAY_LENGTH

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

Done.

+        m_isPreprocessing = false;
+
+        if (!resultValue.IsEmpty() && resultValue->IsString())
+            return toWebCoreStringWithNullCheck(resultValue);
+
+        return sourceCode;
+    }
+
+    bool hasPreprocessorFunction()
+    {
+        return !m_preprocessorFunction.isEmpty();
+    }
+
+    bool isPreprocessing()
+    {
+        return m_isPreprocessing;
+    }
+
+    ~ScriptPreprocessor()
+    {
+    }

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

The compiler will generate this function for you.  You don't need to write it
yourself.

+
+private:
+    String m_preprocessorBody;
+    ScopedPersistent<v8::Function> m_preprocessorFunction;

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

Does this leak memory?

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

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.

+    ScriptController* m_controller;
+    v8::Isolate* m_isolate;
+    bool m_isPreprocessing;
+};
+
+bool ScriptController::hasScriptPreprocessor()
+{
+    return m_scriptPreprocessor && m_scriptPreprocessor->hasPreprocessorFunction();
+}
+
+bool ScriptController::isScriptPreprocessing()
+{
+    return hasScriptPreprocessor() && m_scriptPreprocessor->isPreprocessing();
+}
+
+void ScriptController::setScriptPreprocessor(const String& preprocessorBody)
+{
+    // The preprocessor will be created the first time it is needed.
+    m_scriptPreprocessor.clear();
+    m_preprocessorSource = preprocessorBody;

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

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.

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

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.

+}
+
+String ScriptController::preprocess(const String& scriptSource, const String& scriptName)
+{
+    if (m_preprocessorSource.isEmpty())
+        return scriptSource;
+
+    if (!m_scriptPreprocessor)
+        m_scriptPreprocessor = adoptPtr(new ScriptPreprocessor(m_preprocessorSource, this));
+
+    if (!m_scriptPreprocessor->hasPreprocessorFunction())
+        return scriptSource;
+
+    return m_scriptPreprocessor->preprocessSourceCode(scriptSource, scriptName);

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

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.

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

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

+}
+
+
 } // namespace WebCore