Bug 80992 – Web Inspector: provide a way to reload page with given script preprocessor.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Pavel Feldman]

Downstream bug: http://code.google.com/p/chromium/issues/detail?id=107238

[Pavel Feldman]

Created attachment 131627 [details]
Patch

[WebKit Review Bot]

Comment on attachment 131627 [details]
Patch

Attachment 131627 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11941638

New failing tests:
inspector/debugger/debugger-script-preprocessor.html

[Timothy Hatcher]

Why does this need to be V8/Chrome specific?

[Pavel Feldman]

(In reply to comment #3)
> Why does this need to be V8/Chrome specific?

We'd like to be able to preprocess everything, including eval and new Function. These calls are processed within VM, so need to be handled in a VM-specific manner. Once I land this, I'll look into implementing JSC part and if I fail, I'll file corresponding bug against JSC team.

[Timothy Hatcher]

I see, thanks!

[Timothy Hatcher]

Comment on attachment 131627 [details]
Patch

Please revise the ChangeLog and provide information on what changed and why (overall summary and per function).

[Pavel Feldman]

Created attachment 133806 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 133806 [details]
Patch

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

> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:63
> +    ScriptPreprocessor(const String& preprocessorScript)

Should be explicit.

> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:70
> +        m_utilityContext = v8::Context::New(0, globalTemplate);

Should be just m_utilityContext = v8::Context::New();

> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:74
> +        v8::Local<v8::Context> context = v8::Local<v8::Context>::New(m_utilityContext);

No need to wrap it into a local handle, just pass m_utilityContext to the Scope constructor below.

> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:78
> +        tryCatch.SetVerbose(true);

Note that this wouldn't make any effect as you don't add any message handlers to the utility isolate.

> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:109
> +        tryCatch.SetVerbose(true);

Same here, we may want to propagate the exceptions to the consonle agent.

> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:115
> +            v8::String::Utf8Value utf8Value(resultValue);

Does it always return UTF-8, not UTF-16?

> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:126
> +            if (!m_utilityContext.IsEmpty()) {

You should probably also dispose m_preprocessorFunction here.

> Source/WebCore/bindings/v8/ScriptDebugServer.h:132
> +    static bool s_isCompilingInjectedScript;

This field should be per isolate, not static, otherwise it will break workers.

[Pavel Feldman]

Comment on attachment 133806 [details]
Patch

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

>> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:63
>> +    ScriptPreprocessor(const String& preprocessorScript)
> 
> Should be explicit.

Done.

>> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:70
>> +        m_utilityContext = v8::Context::New(0, globalTemplate);
> 
> Should be just m_utilityContext = v8::Context::New();

Done.

>> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:74
>> +        v8::Local<v8::Context> context = v8::Local<v8::Context>::New(m_utilityContext);
> 
> No need to wrap it into a local handle, just pass m_utilityContext to the Scope constructor below.

Done.

>> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:78
>> +        tryCatch.SetVerbose(true);
> 
> Note that this wouldn't make any effect as you don't add any message handlers to the utility isolate.

Done.

>> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:109
>> +        tryCatch.SetVerbose(true);
> 
> Same here, we may want to propagate the exceptions to the consonle agent.

Done.

>> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:115
>> +            v8::String::Utf8Value utf8Value(resultValue);
> 
> Does it always return UTF-8, not UTF-16?

Utf8Value wrapper is taking care of conversion.

>> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:126
>> +            if (!m_utilityContext.IsEmpty()) {
> 
> You should probably also dispose m_preprocessorFunction here.

Done.

>> Source/WebCore/bindings/v8/ScriptDebugServer.h:132
>> +    static bool s_isCompilingInjectedScript;
> 
> This field should be per isolate, not static, otherwise it will break workers.

Done.

[Pavel Feldman]

Created attachment 133848 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 133848 [details]
Patch

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

> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:80
> +        if (tryCatch.HasCaught())

We should come up with some error reporting mechanism here.

> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:137
> +    String m_preprocessorBody;

This field is unused, please remove.

[johnjbarton]

Created attachment 166718 [details]
Rebase + one fix: ScriptDebugServer::setScriptSource callDebuggerMethod(liveEditScriptSource... because the JS method was renamed

[johnjbarton]

One bit of the rebase I was unsure of:

Source/WebCore/bindings/v8/custom/V8InjectedScriptManager.cpp

V8RecursionScope::MicrotaskSuppression recursionScope; // REVIEW: should this immediately follow Compile?

[Adam Barth]

Comment on attachment 166718 [details]
Rebase + one fix: ScriptDebugServer::setScriptSource callDebuggerMethod(liveEditScriptSource... because the JS method was renamed

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

> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:133
> +            if (!m_preprocessorFunction.IsEmpty()) {
> +                m_preprocessorFunction.Dispose();
> +                m_preprocessorFunction.Clear();
> +            }

Whenever you find yourself writing code like this, please use ScopedPersistent instead.

> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:144
> +    v8::Persistent<v8::Context> m_utilityContext;

Please use ScopedPersistent rather than holding raw persistent handles in member variables.

[Pavel Feldman]

Created attachment 167091 [details]
Patch

[Build Bot]

Comment on attachment 167091 [details]
Patch

Attachment 167091 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14173175

New failing tests:
inspector/debugger/debugger-script-preprocessor.html

[Yury Semikhatsky]

Comment on attachment 167091 [details]
Patch

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

> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:107
> +        RecursionScopeSuppression suppressionScope;

Please use MicrotaskSuppression instead.

> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:149
> +    v8::Isolate* m_utilityIsolate;

You don't seem to use the isolate anymore, please remove this.

> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:447
> +            if (!m_scriptPreprocessor)

You need to protect against evals in the preprocessor script that would trigger recursive BeforeCompile events.

> Source/WebCore/bindings/v8/ScriptDebugServer.h:109
> +    ~ScriptDebugServer();

Please make destructor virtual.

> Source/WebCore/bindings/v8/custom/V8InjectedScriptManager.cpp:99
> +    v8::TryCatch tryCatch;

Please add tryCatch.SetVerbose(true) so that the errors are propagated to the console.

> Source/WebCore/inspector/InspectorDebuggerAgent.h:80
> +    virtual void didClearMainFrameWindowObject();

This method should probably be moved to PageDebuggerAgent completely as there is no such thing as main frame in worker.

> LayoutTests/inspector/debugger/debugger-script-preprocessor.html:23
> +                return script + "//@ sourceURL=window_should_not_be_there";

Please add FAIL word somewhere in this line to make it clear that it is a failure case.

[johnjbarton]

A few more changes are needed to surface the feature for extensions:

diff --git a/Source/WebCore/inspector/front-end/ExtensionServer.js b/Source/WebCore/inspector/front-end/ExtensionServer.js
index 8166e7c..27969a8 100644
--- a/Source/WebCore/inspector/front-end/ExtensionServer.js
+++ b/Source/WebCore/inspector/front-end/ExtensionServer.js
@@ -396,7 +396,8 @@ WebInspector.ExtensionServer.prototype = {
             // returns empty object for compatibility with InjectedScriptManager on the backend.
             injectedScript = "((function(){" + options.injectedScript + "})(),function(){return {}})";
         }
-        PageAgent.reload(!!options.ignoreCache, injectedScript);
+        var preprocessingScript = options.preprocessingScript;
+        PageAgent.reload(!!options.ignoreCache, injectedScript, preprocessingScript);
         return this._status.OK();
     },
 
diff --git a/Source/WebCore/inspector/front-end/externs.js b/Source/WebCore/inspector/front-end/externs.js
index ba5bbc9..9135a8e 100644
--- a/Source/WebCore/inspector/front-end/externs.js
+++ b/Source/WebCore/inspector/front-end/externs.js
@@ -253,6 +253,7 @@ function ExtensionDescriptor() {
 function ExtensionReloadOptions() {
     this.ignoreCache = false;
     this.injectedScript = "";
+    this.preprocessingScript = "";
     this.userAgent = "";
 }

[johnjbarton]

Some feedback from trying this patch in my app:

  1) We need errors in the preprocessing step to show up; they currently just vanish and as far as I can tell the un-processed script proceeds.

  2) The preprocessing script will need environmental metadata, eg the script tag attributes and the page URL. The key use case will be transcoding and we'll want to build sourcemaps for backcoding. These sourcemaps need the script tag filename.

  3) The biggest plus of this approach is synchrony with the browser runtime: the transcoded output runs in the same order as the original source; the downside is that the only communications path (I guess) between the transcoder and other system components is the source output. So, for example, development tools relying on syntax trees will need to encode a copy of the original source in a comment, then re-compile it outside of the web page.  If the preprocessingScript were a bit more like chrome extension content-scripts or web workers, we could leave the state in the world of the preprocessingScript then query it with postMessage() for tools. Ok maybe that's asking a lot ;-).

[johnjbarton]

Oops, sorry I spoke too soon.

With a preprocessing script like:

  function transcode(str) {
    return  str + "\nconsole.log('transcoded by preprocessingScript');\n//@ sourceURL=fake_url"+Math.random();
  }

and a script tag:

 <script>
  function transcodeMe() {}
  </script>

The console output is

with ((window && window.console && window.console._commandLineAPI) || {}) {
(function getScripts() { // runs in the web page
    var scriptElements = document.querySelectorAll('script');
    var scripts = [];
    for(var i = 0; i < scriptElements.length; i++) {
      var elt = scriptElements[i];
      //console.log("scripts["+i+"/"+scriptElements.length+"] "+elt.src);
      scripts.push({
        src: elt.src,
        textContent: elt.textContent
      });
    }

    return scripts;
  }([]));
}
console.log('transcoded by preprocessingScript');
//@ sourceURL=fake_url0.6286467798054218

getScripts is a function I injected with chrome.devtools.inspectedWindow.eval

So somehow the signals are getting crossed....

[Pavel Feldman]

Created attachment 167994 [details]
Patch

[Pavel Feldman]

Comment on attachment 167091 [details]
Patch

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

>> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:107
>> +        RecursionScopeSuppression suppressionScope;
> 
> Please use MicrotaskSuppression instead.

I intentionally used specific suppressor.

>> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:149
>> +    v8::Isolate* m_utilityIsolate;
> 
> You don't seem to use the isolate anymore, please remove this.

Done.

>> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:447
>> +            if (!m_scriptPreprocessor)
> 
> You need to protect against evals in the preprocessor script that would trigger recursive BeforeCompile events.

Done.

>> Source/WebCore/bindings/v8/ScriptDebugServer.h:109
>> +    ~ScriptDebugServer();
> 
> Please make destructor virtual.

Done.

>> Source/WebCore/bindings/v8/custom/V8InjectedScriptManager.cpp:99
>> +    v8::TryCatch tryCatch;
> 
> Please add tryCatch.SetVerbose(true) so that the errors are propagated to the console.

Done.

>> Source/WebCore/inspector/InspectorDebuggerAgent.h:80
>> +    virtual void didClearMainFrameWindowObject();
> 
> This method should probably be moved to PageDebuggerAgent completely as there is no such thing as main frame in worker.

Done.

>> LayoutTests/inspector/debugger/debugger-script-preprocessor.html:23
>> +                return script + "//@ sourceURL=window_should_not_be_there";
> 
> Please add FAIL word somewhere in this line to make it clear that it is a failure case.

Done.

[Yury Semikhatsky]

Comment on attachment 167994 [details]
Patch

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

Please file bug on the issues raised by John.

> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:122
> +        v8::Local<v8::Context> context = v8::Local<v8::Context>::New(m_utilityContext.get());

You should check for empty m_utilityContext here.

> Source/WebCore/inspector/Inspector.json:227
> +                    { "name": "scriptPreprocessor", "type": "string", "optional": true, "description": "Script body that should evaluate to function that will preprocess all the scripts before their compilation.", "hidden": true }

Please describe the function signature.

[Pavel Feldman]

> So somehow the signals are getting crossed....

I think this works as intended (at least as coded). Pre-processor processes all the scripts before they get into VM on the lowest level possible.

[johnjbarton]

(In reply to comment #24)
> > So somehow the signals are getting crossed....
> 
> I think this works as intended (at least as coded). Pre-processor processes all the scripts before they get into VM on the lowest level possible.

Then I don't understand how to use this feature:
1) It does not seem to preprocess script tags in the page that I want to preprocess and 
2) it does preprocess code I  add to work with the preprocessing system (and thus do not want preprocessed). I can puts some sentinel in every eval I guess, seems hacky.

[johnjbarton]

The patch from 10/10 needs a one function call rename in bindings/v8/ScriptDebugServer.cpp line 454 for
https://bugs.webkit.org/show_bug.cgi?id=103013

[Pavel Feldman]

Created attachment 177630 [details]
[Patch] rebaselined

[Pavel Feldman]

Committed r136949: <http://trac.webkit.org/changeset/136949>

[johnjbarton]

The new test files intended for this bug are now on bug 104384

[Vsevolod Vlasov]

This patch breaks Debugger domain.
GlobalObjectCleared event is not dispatched anymore when navigation within single renderer occurs.
See https://bugs.webkit.org/show_bug.cgi?id=106543