RESOLVED FIXED 80992
Web Inspector: provide a way to reload page with given script preprocessor.
https://bugs.webkit.org/show_bug.cgi?id=80992
Summary Web Inspector: provide a way to reload page with given script preprocessor.
Pavel Feldman
Reported 2012-03-13 08:51:26 PDT
Attachments
Patch (28.62 KB, patch)
2012-03-13 08:53 PDT, Pavel Feldman
no flags
Patch (29.96 KB, patch)
2012-03-26 07:27 PDT, Pavel Feldman
no flags
Patch (29.73 KB, patch)
2012-03-26 11:04 PDT, Pavel Feldman
no flags
Rebase + one fix: ScriptDebugServer::setScriptSource callDebuggerMethod(liveEditScriptSource... because the JS method was renamed (31.29 KB, patch)
2012-10-02 11:19 PDT, johnjbarton
no flags
Patch (31.75 KB, patch)
2012-10-04 06:04 PDT, Pavel Feldman
no flags
Patch (36.61 KB, patch)
2012-10-10 07:10 PDT, Pavel Feldman
no flags
[Patch] rebaselined (33.52 KB, patch)
2012-12-04 18:01 PST, Pavel Feldman
no flags
Pavel Feldman
Comment 1 2012-03-13 08:53:40 PDT
WebKit Review Bot
Comment 2 2012-03-13 09:45:17 PDT
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
Comment 3 2012-03-13 10:07:58 PDT
Why does this need to be V8/Chrome specific?
Pavel Feldman
Comment 4 2012-03-13 10:52:01 PDT
(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
Comment 5 2012-03-13 10:58:40 PDT
I see, thanks!
Timothy Hatcher
Comment 6 2012-03-21 13:00:41 PDT
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
Comment 7 2012-03-26 07:27:12 PDT
Yury Semikhatsky
Comment 8 2012-03-26 08:39:47 PDT
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 9 2012-03-26 11:03:28 PDT
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
Comment 10 2012-03-26 11:04:57 PDT
Yury Semikhatsky
Comment 11 2012-03-27 04:46:04 PDT
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
Comment 12 2012-10-02 11:19:30 PDT
Created attachment 166718 [details] Rebase + one fix: ScriptDebugServer::setScriptSource callDebuggerMethod(liveEditScriptSource... because the JS method was renamed
johnjbarton
Comment 13 2012-10-02 11:22:08 PDT
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 14 2012-10-02 12:23:37 PDT
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
Comment 15 2012-10-04 06:04:14 PDT
Build Bot
Comment 16 2012-10-04 07:38:27 PDT
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 17 2012-10-04 09:11:44 PDT
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
Comment 18 2012-10-09 14:36:09 PDT
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
Comment 19 2012-10-09 15:01:11 PDT
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
Comment 20 2012-10-09 15:21:49 PDT
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
Comment 21 2012-10-10 07:10:50 PDT
Pavel Feldman
Comment 22 2012-10-11 06:53:14 PDT
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 23 2012-10-11 07:03:25 PDT
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
Comment 24 2012-10-11 09:26:02 PDT
> 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
Comment 25 2012-10-11 14:41:52 PDT
(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
Comment 26 2012-12-04 10:35:22 PST
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
Comment 27 2012-12-04 18:01:39 PST
Created attachment 177630 [details] [Patch] rebaselined
Pavel Feldman
Comment 28 2012-12-07 06:14:33 PST
johnjbarton
Comment 29 2012-12-07 12:13:56 PST
The new test files intended for this bug are now on bug 104384
Vsevolod Vlasov
Comment 30 2013-01-10 02:16:25 PST
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
Note You need to log in before you can comment on or make changes to this bug.