Downstream bug: http://code.google.com/p/chromium/issues/detail?id=107238
Created attachment 131627 [details] Patch
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
Why does this need to be V8/Chrome specific?
(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.
I see, thanks!
Comment on attachment 131627 [details] Patch Please revise the ChangeLog and provide information on what changed and why (overall summary and per function).
Created attachment 133806 [details] Patch
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.
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.
Created attachment 133848 [details] Patch
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.
Created attachment 166718 [details] Rebase + one fix: ScriptDebugServer::setScriptSource callDebuggerMethod(liveEditScriptSource... because the JS method was renamed
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?
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.
Created attachment 167091 [details] Patch
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
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.
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 = ""; }
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 ;-).
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....
Created attachment 167994 [details] Patch
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.
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.
> 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.
(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.
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
Created attachment 177630 [details] [Patch] rebaselined
Committed r136949: <http://trac.webkit.org/changeset/136949>
The new test files intended for this bug are now on bug 104384
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