Bug 27136

Summary:

[Qt] Webkit hangs when executing an infinite JavaScript loop

Product:

WebKit

Reporter:

Yael <yael>

Component:

WebKit Qt

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

hausmann, laszlo.gombos, zecke

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Add UI asking the user if to stop running the script	zecke: review-
Replace virtual method with signal/slot	none
Replace the signal with QMetaObject::invokeMethod	hausmann: review-
Added test case	zecke: review+

Yael

Reported 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

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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Yael

Comment 1 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.

Holger Freyther

Comment 2 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.

Yael

Comment 3 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 :-)

Yael

Comment 4 2009-07-10 11:59:49 PDT

Created attachment 32568 [details] Replace the signal with QMetaObject::invokeMethod Third time is a charm :-)

Simon Hausmann

Comment 5 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

Yael

Comment 6 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.

Holger Freyther

Comment 7 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.

Holger Freyther

Comment 8 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...

Yael

Comment 9 2009-07-10 18:57:23 PDT

Landed in 45731.

Note You need to log in before you can comment on or make changes to this bug.