Javascript backtrace support #221

Closed
wants to merge 6 commits into
from

Projects

None yet

2 participants

@jonleighton
Contributor

This is a WIP pull request to discuss my backtraces patch.

It's unfinished, but currently works to an extent:

$ cat test.js 
function a() {
  foo
}

a()
$ phantomjs test.js 
ReferenceError: Can't find variable: foo

  test.js:2 in a
  test.js:5

I will comment more tomorrow when I have time, right now I need to sleep.

Owner
ariya commented Mar 15, 2012

Meanwhile, I cherry-pick the .gitignore commit.

Contributor

Ok, here is my current TODO list:

  • Output on STDERR
  • Ensure errors thrown by loaded pages (i.e. page.open(...)) work properly. They should report to the page's onError handler. (This may work, I just haven't checked yet.)
  • Ensure injectJs() code reports errors correctly
  • Ensure the QWebPagePrivateDebugger instance is destroyed when the page is destroyed.
  • Rename QWebPagePrivateDebugger to QWebPagePrivate::Debugger
  • Check / write tests
  • Look into whether we can pass a string as the source file, rather than having to use a QUrl. Would allow for a bit more flexibility.

Other things:

  • Parse errors - lower priority, but QWebPagePrivateDebugger::sourceParsed could report these.
  • page.evaluate() - this is a tricky one.

Regarding page.evaluate, currently our WebPage::evaluate method receives a QString. As it is, this is going to make it impossible to correctly report errors within the page.evaluate. For example:

var page = require('webpage').create()
var foo = "foo\nbar\n"
foo += "baz"
page.evaluate(foo)

The string simply does not provide enough information to know the original source line for the error. Even if we pass a function to page.evaluate, that gets converted to a string by the QtWebKit bridge.

I am not sure it will be possible to make the string case work - certainly not before 1.5, but I should think/investigate a bit more about the page.evaluate(function() { ... }) case and see if we can get any useful information out of that.

We could also make the assumption that the evaluated code starts at the line of the page.evaluate call - I guess that assumption would be correct 80% of the time, so would be better than nothing.

@jonleighton jonleighton commented on the diff Mar 15, 2012
src/phantom.cpp
@@ -114,8 +114,6 @@
m_page->applySettings(m_defaultPageSettings);
setLibraryPath(QFileInfo(m_config.scriptFile()).dir().absolutePath());
-
- onInitialized();
jonleighton
jonleighton Mar 15, 2012 Contributor

onInitialized was getting called twice, meaning that the onError handler got connected twice. So I removed this line. Was there a reason for it being here though?

@jonleighton jonleighton commented on the diff Mar 15, 2012
...ebkit/Source/WebCore/bindings/js/ScriptController.cpp
@@ -154,9 +154,6 @@ ScriptValue ScriptController::evaluateInWorld(const ScriptSourceCode& sourceCode
return ScriptValue(exec->globalData(), comp.value());
}
- if (comp.complType() == Throw || comp.complType() == Interrupted)
- reportException(exec, comp.value());
jonleighton
jonleighton Mar 15, 2012 Contributor

This stops exceptions being reported to QWebPage::javaScriptConsoleMessage, as we are using the debugger to monitor the call stack and report exceptions separately.

@jonleighton jonleighton commented on an outdated diff Mar 15, 2012
...rc/3rdparty/webkit/Source/WebKit/qt/Api/qwebframe.cpp
@@ -1529,14 +1529,14 @@ void QWebFrame::print(QPrinter *printer) const
\sa addToJavaScriptWindowObject(), javaScriptWindowObjectCleared()
*/
-QVariant QWebFrame::evaluateJavaScript(const QString& scriptSource)
+QVariant QWebFrame::evaluateJavaScript(const QString& scriptSource, const QUrl& file)
jonleighton
jonleighton Mar 15, 2012 Contributor

I added this as a required parameter to ensure that if we are providing no source file, then it is done explicitly. (i.e. for small snippets of internal code)

@jonleighton jonleighton commented on the diff Mar 15, 2012
...src/3rdparty/webkit/Source/WebKit/qt/Api/qwebpage.cpp
+
+QWebPagePrivateDebugger::~QWebPagePrivateDebugger()
+{
+}
+
+void QWebPagePrivateDebugger::detach(JSC::JSGlobalObject*)
+{
+}
+
+void QWebPagePrivateDebugger::sourceParsed(JSC::ExecState*, JSC::SourceProvider*, int errorLineNumber, const JSC::UString& errorMessage)
+{
+}
+
+void QWebPagePrivateDebugger::exception(const JSC::DebuggerCallFrame& frame, intptr_t sourceID, int lineNumber, bool hasHandler)
+{
+ if (!hasHandler) {
jonleighton
jonleighton Mar 15, 2012 Contributor

This filter out exceptions that are caught by a catch statement.

@jonleighton jonleighton commented on the diff Mar 15, 2012
...src/3rdparty/webkit/Source/WebKit/qt/Api/qwebpage.cpp
+ QList<QWebPage::JavaScriptFrame> qbacktrace;
+
+ JSC::SourceProvider* provider;
+
+ QString file;
+ int line;
+ QString function;
+
+ while (frame) {
+ provider = reinterpret_cast<JSC::SourceProvider*>(frame->sourceID());
+
+ line = frame->line();
+ file = QString::fromUtf8(provider->url().utf8().data());
+ function = QString::fromUtf8(frame->functionName().utf8().data());
+
+ qbacktrace << QWebPage::JavaScriptFrame(file, line + 1, function);
jonleighton
jonleighton Mar 15, 2012 Contributor

line is zero indexed, hence the + 1

@jonleighton jonleighton commented on an outdated diff Mar 15, 2012
...src/3rdparty/webkit/Source/WebKit/qt/Api/qwebpage.cpp
@@ -370,6 +472,8 @@ static inline Qt::DropAction dragOpToDropAction(unsigned actions)
#if ENABLE(NOTIFICATIONS)
NotificationPresenterClientQt::notificationPresenter()->addClient();
#endif
+
+ page->setDebugger(new QWebPagePrivateDebugger(q));
jonleighton
jonleighton Mar 15, 2012 Contributor

Note that a page can only have one debugger instance; if the web inspector is enabled, this will get replaced. But that seems okay, as if you enable the web inspector you're presumably using that for debugging instead of terminal output.

Contributor

Rebased and pushed some more commits. I've left them separate so you can see what changed, but we can squash at the end.

External webpages and stuff loaded via injectJs works fine.

Here's what webpage.evaluate() produces currently:

$ cat test.js 
page = require('webpage').create();

page.evaluate(function() {
  function omg() {
    foo
  }

  omg()
})

$ phantomjs test.js 
ReferenceError: Can't find variable: foo

  phantomjs://webpage.evaluate():3 in omg
  phantomjs://webpage.evaluate():6
  phantomjs://webpage.evaluate():1
  test.js:3

I think this is the best we can do for the 1.5 release. We should explicitly encourage people to use injectJs if they want to put larger bits of code into a page, as that will allow for proper error reporting.

Owner
ariya commented Mar 16, 2012

Looks good enough for 1.5. I'll cherry-pick it, I will likely separate the change to src/qt so that it can be cherry-picked again in the future once Qt 4.8.1 is imported.

Don't worry about stderr. Ideally we use console.error though right now it does not properly (I think there is a filed ticket about that?).

Same with the rest of the machinery. Various endless level of wrapping is likely gone once we plan to cleanup the QtWebKit bridge. Since we have the code at our disposal, this should be easier to handle, possible right at the JS engine level. But this is 1.6 materials.

Thanks a lot for your effort!

Owner
ariya commented Mar 18, 2012

Merged. Thanks!

@ariya ariya closed this Mar 18, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment