Bug 111889 - Web Inspector: Error handling for ScriptPreprocessor
Summary: Web Inspector: Error handling for ScriptPreprocessor
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-08 14:36 PST by johnjbarton
Modified: 2014-12-11 09:24 PST (History)
19 users (show)

See Also:


Attachments
Patch (6.27 KB, patch)
2013-03-08 14:43 PST, johnjbarton
no flags Details | Formatted Diff | Diff
Patch (16.49 KB, patch)
2013-03-19 16:52 PDT, johnjbarton
no flags Details | Formatted Diff | Diff
Fix 1/2 issues from comment #13; move -expected.txt file to the correct location (17.16 KB, patch)
2013-03-20 09:48 PDT, johnjbarton
no flags Details | Formatted Diff | Diff
Add name for preprocessor script so we get line numbers in error messages; fix bindings/js for other platform builds (18.32 KB, patch)
2013-03-20 16:58 PDT, johnjbarton
no flags Details | Formatted Diff | Diff
Fix signature for mac builds (18.30 KB, patch)
2013-03-22 15:26 PDT, johnjbarton
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (487.67 KB, application/zip)
2013-03-22 20:42 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-06 for mac-future (922.97 KB, application/zip)
2013-03-23 05:49 PDT, Build Bot
no flags Details
Try again on mac port expected.txt (18.83 KB, patch)
2013-03-27 15:31 PDT, johnjbarton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description johnjbarton 2013-03-08 14:36:21 PST
In using the scriptpreprocessor introduced in Bug 80992 and Bug 104384, I found I could succeed with trivial preprocessors but not real JS to JS compilers.

The problem is two-fold: 
1) the environment of the preprocessor is not like any web page: the compiler source must be inside of a function body, not a global scope and the compiler cannot reference console or window.
2) Compile and run time errors are not reported, but result in silently ignoring the preprocessor.

Discovering the first set of issues without the second is pretty much impossible.
Comment 1 johnjbarton 2013-03-08 14:43:15 PST
Created attachment 192286 [details]
Patch
Comment 2 johnjbarton 2013-03-08 14:46:59 PST
By adding some LOG_ERROR calls I was able to succeed with my realistic preprocessor (based on traceur JS to JS compiler). 

The next step would plumb the errors back to JS land on the client side, but I don't know how to do that since the errors are asynchronous to the reload() call. For this purpose I think sending console errors back to the debugger would be fine, also something I don't know how to do.
Comment 3 johnjbarton 2013-03-19 16:52:15 PDT
Created attachment 193950 [details]
Patch
Comment 4 Early Warning System Bot 2013-03-19 16:57:17 PDT
Comment on attachment 193950 [details]
Patch

Attachment 193950 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17179419
Comment 5 Early Warning System Bot 2013-03-19 16:57:52 PDT
Comment on attachment 193950 [details]
Patch

Attachment 193950 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17139606
Comment 6 Build Bot 2013-03-19 17:13:58 PDT
Comment on attachment 193950 [details]
Patch

Attachment 193950 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17191462
Comment 7 Build Bot 2013-03-19 17:15:43 PDT
Comment on attachment 193950 [details]
Patch

Attachment 193950 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17190644
Comment 8 kov's GTK+ EWS bot 2013-03-19 17:20:21 PDT
Comment on attachment 193950 [details]
Patch

Attachment 193950 [details] did not pass gtk-ews (gtk):
Output: http://webkit-commit-queue.appspot.com/results/17129670
Comment 9 EFL EWS Bot 2013-03-19 17:55:45 PDT
Comment on attachment 193950 [details]
Patch

Attachment 193950 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17139615
Comment 10 WebKit Review Bot 2013-03-19 19:14:25 PDT
Comment on attachment 193950 [details]
Patch

Attachment 193950 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17139633

New failing tests:
inspector/debugger/debugger-script-preprocessor.html
Comment 11 WebKit Review Bot 2013-03-19 19:49:01 PDT
Comment on attachment 193950 [details]
Patch

Attachment 193950 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17066874

New failing tests:
inspector/debugger/debugger-script-preprocessor.html
Comment 12 Build Bot 2013-03-19 23:31:22 PDT
Comment on attachment 193950 [details]
Patch

Attachment 193950 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17125705
Comment 13 Pavel Feldman 2013-03-20 00:58:37 PDT
Comment on attachment 193950 [details]
Patch

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

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

Did you try calling SetVerbose on the handler instead of all of this code?

> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:123
> +            exceptionTarget->document()->reportException("preprocessor script must evaluate to a function", 0, "", 0);

Please do not reach for the console via document - there is now PageConsole accessible from page. You might need to be Page-aware (PageScriptDebugServer)

> Source/WebCore/bindings/v8/V8Binding.cpp:88
> +    String exceptionMessage = toWebCoreStringWithUndefinedOrNullCheck(message->Get());

I don't think we need another method of a kind. Certainly not here.
Comment 14 johnjbarton 2013-03-20 07:35:08 PDT
(In reply to comment #13)
> (From update of attachment 193950 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193950&action=review
> 
> > Source/WebCore/bindings/v8/ScriptDebugServer.cpp:109
> > +        if (tryCatch.HasCaught()) {
> 
> Did you try calling SetVerbose on the handler instead of all of this code?

Setting tryCatch.setVerbose(true) triggers WebKit/Source/WebCore/bindings/v8/V8Initializer messageHandlerInMainThread, a message listener on v8::V8.  The messageHandlerInMainThread() only works if called on a context related to the main thread window. The preprocessor however is in a utility context and has no 'window' so messageHandlerInMainThread() crashes.  Since this handler is hard wired into the thread, there is no practical way to replace it. 

> 
> > Source/WebCore/bindings/v8/ScriptDebugServer.cpp:123
> > +            exceptionTarget->document()->reportException("preprocessor script must evaluate to a function", 0, "", 0);
> 
> Please do not reach for the console via document - there is now PageConsole accessible from page. You might need to be Page-aware (PageScriptDebugServer)

I don't understand what you mean by Page-aware or Page for that matter. But I can remove the path though document().

> 
> > Source/WebCore/bindings/v8/V8Binding.cpp:88
> > +    String exceptionMessage = toWebCoreStringWithUndefinedOrNullCheck(message->Get());
> 
> I don't think we need another method of a kind. Certainly not here.

Sorry I don't understand "method of a kind"? To be sure, I don't know which conversion from Message to String is appropriate here, I just used one that was already used in the file. What should I use instead?
Comment 15 johnjbarton 2013-03-20 09:48:05 PDT
Created attachment 194082 [details]
Fix 1/2 issues from comment #13; move -expected.txt file to the correct location
Comment 16 Early Warning System Bot 2013-03-20 09:57:03 PDT
Comment on attachment 194082 [details]
Fix 1/2 issues from comment #13; move -expected.txt file to the correct location

Attachment 194082 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17155737
Comment 17 Early Warning System Bot 2013-03-20 09:58:19 PDT
Comment on attachment 194082 [details]
Fix 1/2 issues from comment #13; move -expected.txt file to the correct location

Attachment 194082 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17286056
Comment 18 kov's GTK+ EWS bot 2013-03-20 09:59:04 PDT
Comment on attachment 194082 [details]
Fix 1/2 issues from comment #13; move -expected.txt file to the correct location

Attachment 194082 [details] did not pass gtk-ews (gtk):
Output: http://webkit-commit-queue.appspot.com/results/17190723
Comment 19 EFL EWS Bot 2013-03-20 10:01:13 PDT
Comment on attachment 194082 [details]
Fix 1/2 issues from comment #13; move -expected.txt file to the correct location

Attachment 194082 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17178671
Comment 20 Build Bot 2013-03-20 10:07:06 PDT
Comment on attachment 194082 [details]
Fix 1/2 issues from comment #13; move -expected.txt file to the correct location

Attachment 194082 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17155742
Comment 21 Build Bot 2013-03-20 10:46:14 PDT
Comment on attachment 194082 [details]
Fix 1/2 issues from comment #13; move -expected.txt file to the correct location

Attachment 194082 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17232360
Comment 22 Build Bot 2013-03-20 14:43:06 PDT
Comment on attachment 194082 [details]
Fix 1/2 issues from comment #13; move -expected.txt file to the correct location

Attachment 194082 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17196627
Comment 23 Build Bot 2013-03-20 15:14:43 PDT
Comment on attachment 194082 [details]
Fix 1/2 issues from comment #13; move -expected.txt file to the correct location

Attachment 194082 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17136780
Comment 24 johnjbarton 2013-03-20 16:58:10 PDT
Created attachment 194146 [details]
Add name for preprocessor script so we get line numbers in error messages; fix bindings/js for other platform builds
Comment 25 johnjbarton 2013-03-20 17:08:00 PDT
I worked with this patch today. I was able to use some of the error messages to fix some of my problems. 

The remaining problem is related to how the Inspector works. If the preprocessor returns a results with a syntax error, then the browser's JS compiler never sees the //@ sourceURL. So the compilation is associated with the original source URL but we don't get line numbers (I guess because the compiler sees no name at all). The result is confusing: an error message against the original source which has no error. 

(This case is more common than you might think because it just means the preprocessor returned a string that is not JS).

Somehow the preprocessor output is not registered with the Inspector in this case.

(I also hit a problem not directly related to error processing: Inspector evaluations also pass thru the preprocessor (surprise!). My transcoder did not deal with "with" at top level, so I ended up in the case above.)
Comment 26 Build Bot 2013-03-20 17:33:29 PDT
Comment on attachment 194146 [details]
Add name for preprocessor script so we get line numbers in error messages; fix bindings/js for other platform builds

Attachment 194146 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17216473
Comment 27 Build Bot 2013-03-21 00:20:59 PDT
Comment on attachment 194146 [details]
Add name for preprocessor script so we get line numbers in error messages; fix bindings/js for other platform builds

Attachment 194146 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17258152
Comment 28 johnjbarton 2013-03-22 15:26:22 PDT
Created attachment 194643 [details]
Fix signature for mac builds
Comment 29 Build Bot 2013-03-22 20:41:59 PDT
Comment on attachment 194643 [details]
Fix signature for mac builds

Attachment 194643 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17207680

New failing tests:
inspector/debugger/debugger-script-preprocessor.html
Comment 30 Build Bot 2013-03-22 20:42:02 PDT
Created attachment 194687 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2
Comment 31 Build Bot 2013-03-23 05:49:02 PDT
Comment on attachment 194643 [details]
Fix signature for mac builds

Attachment 194643 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17240440

New failing tests:
svg/batik/filters/feTile.svg
inspector/debugger/debugger-script-preprocessor.html
Comment 32 Build Bot 2013-03-23 05:49:08 PDT
Created attachment 194700 [details]
Archive of layout-test-results from webkit-ews-06 for mac-future

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-future  Platform: Mac OS X 10.8.2
Comment 33 johnjbarton 2013-03-27 15:31:34 PDT
Created attachment 195413 [details]
Try again on mac port expected.txt
Comment 34 Brian Burg 2014-12-11 09:24:54 PST
Current inspector doesn't have preprocessor.