WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Downstream bug:
http://code.google.com/p/chromium/issues/detail?id=107238
Attachments
Patch
(28.62 KB, patch)
2012-03-13 08:53 PDT
,
Pavel Feldman
no flags
Details
Formatted Diff
Diff
Patch
(29.96 KB, patch)
2012-03-26 07:27 PDT
,
Pavel Feldman
no flags
Details
Formatted Diff
Diff
Patch
(29.73 KB, patch)
2012-03-26 11:04 PDT
,
Pavel Feldman
no flags
Details
Formatted Diff
Diff
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
Details
Formatted Diff
Diff
Patch
(31.75 KB, patch)
2012-10-04 06:04 PDT
,
Pavel Feldman
no flags
Details
Formatted Diff
Diff
Patch
(36.61 KB, patch)
2012-10-10 07:10 PDT
,
Pavel Feldman
no flags
Details
Formatted Diff
Diff
[Patch] rebaselined
(33.52 KB, patch)
2012-12-04 18:01 PST
,
Pavel Feldman
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Pavel Feldman
Comment 1
2012-03-13 08:53:40 PDT
Created
attachment 131627
[details]
Patch
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
Created
attachment 133806
[details]
Patch
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
Created
attachment 133848
[details]
Patch
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
Created
attachment 167091
[details]
Patch
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
Created
attachment 167994
[details]
Patch
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
Committed
r136949
: <
http://trac.webkit.org/changeset/136949
>
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.
Top of Page
Format For Printing
XML
Clone This Bug