Bug 27136 - [Qt] Webkit hangs when executing an infinite JavaScript loop
Summary: [Qt] Webkit hangs when executing an infinite JavaScript loop
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-09 16:38 PDT by Yael
Modified: 2009-07-10 18:57 PDT (History)
3 users (show)

See Also:


Attachments
Add UI asking the user if to stop running the script (2.98 KB, patch)
2009-07-09 16:45 PDT, Yael
zecke: review-
Details | Formatted Diff | Diff
Replace virtual method with signal/slot (3.76 KB, patch)
2009-07-10 10:00 PDT, Yael
no flags Details | Formatted Diff | Diff
Replace the signal with QMetaObject::invokeMethod (2.70 KB, patch)
2009-07-10 11:59 PDT, Yael
hausmann: review-
Details | Formatted Diff | Diff
Added test case (4.04 KB, patch)
2009-07-10 13:47 PDT, Yael
zecke: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yael 2009-07-09 16:38:53 PDT
When executing an infinite loop in JavaScript, the Browser hangs. That is because the built-in toimeout mechanism is not exposed through the Qt API.
The following JavaScript can cause the crash:

<html>
<head>
<script type="text/javascript">
var run = true;
var a = 1;
function startLoop(){
    while(run){
        a++;
        document.forms[0].elements[0].value=a;
    }	
}  
</script>
</head><body>
Activate the "StartLoop" link below to start an infinite loop by script execution.<br>
<a href="#" onclick="startLoop()">Start Loop</a><br>
<form><input></form>
</body>
</html>


A patch will be uploaded shortly
Comment 1 Yael 2009-07-09 16:45:17 PDT
Created attachment 32539 [details]
Add UI asking the user if to stop running the script

Following the model of ChromeClientQt::runJavaScriptConfirm(), added a callback to QWebPage, to display a query to the user.
Comment 2 Holger Freyther 2009-07-10 07:35:06 PDT
Comment on attachment 32539 [details]
Add UI asking the user if to stop running the script

Yael, we may not add new virtual methods as this will change the vtable and break ABI to previous versions. One common way is to create a new method with return value and use QMetaObject::invokeMethod. I assume Qt Software has an internal document describing what to do now. Besides that the patch looks fine.
Comment 3 Yael 2009-07-10 10:00:58 PDT
Created attachment 32561 [details]
Replace virtual method with signal/slot

I followed the instructions from http://techbase.kde.org/Policies/Binary_Compatibility_Examples#Add_new_virtuals_to_a_non-leaf_class.

I hope I got it right this time :-)
Comment 4 Yael 2009-07-10 11:59:49 PDT
Created attachment 32568 [details]
Replace the signal with QMetaObject::invokeMethod

Third time is a charm :-)
Comment 5 Simon Hausmann 2009-07-10 12:08:50 PDT
Comment on attachment 32568 [details]
Replace the signal with QMetaObject::invokeMethod

> Index: WebKit/qt/WebCoreSupport/ChromeClientQt.cpp
> ===================================================================
> --- WebKit/qt/WebCoreSupport/ChromeClientQt.cpp	(revision 45709)
> +++ WebKit/qt/WebCoreSupport/ChromeClientQt.cpp	(working copy)
> @@ -288,8 +288,7 @@
>  
>  bool ChromeClientQt::shouldInterruptJavaScript()
>  {
> -    notImplemented();
> -    return false;
> +    return QMetaObject::invokeMethod(m_webPage, "shouldInterruptJavaScript", Qt::DirectConnection);

I don't think this is correct, invokeMethod returns a boolean indicating whether the function could be called or not, but the return value itself is placed in the fourth argument of invokeMethod.

Also a unit test would be good for this :)

An alternate solution would be to use QWebPage::supportsExtension and QWebPage::extension. Holger, do you have any preference?





I
Comment 6 Yael 2009-07-10 13:47:30 PDT
Created attachment 32576 [details]
Added test case

I am thinking that using the extension is probably an overkill for this feature. It is very similar to JavaScript alert implementation. However, if there is a strong opinion otherwise, I will modify it again.
Comment 7 Holger Freyther 2009-07-10 14:44:23 PDT
Comment on attachment 32576 [details]
Added test case

+ JSTestPage(QObject* parent = 0) : QWebPage(parent) {}

one style comment, could move the initializer list to the next line in accordance with the style guidelines?


> +public slots:

please make that public Q_SLOTS... the reason is that it could work with boost and other things.
  
>  /*!
> +    This function is called when a JavaScript program is running for a long period of time.
> +
> +    If the user wanted to stop the JavaScript the implementation should return true; otherwise false.
> +
> +    The default implementation executes the query using QMessageBox::information with QMessageBox::Yes and QMessageBox::No buttons.



We need to add a @since 4.6 tag to the documentation. And please add some comments to make that virtual in the future? basically it is r=me when you change the two when landing.
Comment 8 Holger Freyther 2009-07-10 14:48:22 PDT
Comment on attachment 32576 [details]
Added test case

Yael wants to fix it before landing. So r=+ for me. We might have to discuss on monday if extensions are better...
Comment 9 Yael 2009-07-10 18:57:23 PDT
Landed in 45731.