For Qt port of WebKit: Go to a page that makes use of HTML5 Local Storage Store some data in Local Storage Quit the app ==> Bug: the Local Storage file remains open up to the moment the application quits. On the Mac, _applicationWillTerminate() in WebView.mm calls PageGroup::closeLocalStorage(). On Windows, shutDownWebKit() in WebKitDLL.cpp calls WebCore::PageGroup::closeLocalStorage(). The QWebView should probably listen to an exit signal and call PageGroup::closeLocalStorage(). I will add patch very soon...
I was asked to create this error, which originated in Qt error database. On testing my patch however on linux it seems to be of little value. I see local storage files being closed from destructor even though PageGroup::closeLocalStorage() not being called. Was testing to see if calling PageGroup::closeLocalStorage() on QCoreApplication signal aboutToQuit was of value in some shutdown situation but had problems receiving aboutToQuit signal.... There is Qt err concerning not receiving aboutToQuit signal. Perhaps will try to test on symbian........
Created attachment 90937 [details] Small test to test local storage
Created attachment 91169 [details] Close Icon and Local Storage Databases on Quit Added QWebView::shutDownWebKit to close the Icon and Local Storage Databases. This Signal was connected to the application's aboutToQuit Slot. connect(QCoreApplication::instance(), SIGNAL(aboutToQuit()), this, SLOT(shutDownWebKit())); This was suggested in the error and as is done on other platforms (Windows). Then I connected the Signal lastWindowClosed to Slot quit(), so aboutToQuit would be signaled. connect(QApplication::instance(), SIGNAL(lastWindowClosed()), QApplication::instance(), SLOT(quit())); It turns out that calling WebCore::PageGroup::closeLocalStorage() does not close the local storage. Local storage is closed the WebCore::PageGroup is destroyed. I kept the call to closeLocalStorage() because it was consistent with other platforms and does not hurt anything. Calling WebCore::iconDatabase()->close() in shutDownWebKit does close that db. This was clean up which did not change any functionality, so did not add any new tests.
Attachment 91169 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/Api/qwebview.cpp', u'Sour..." exit_code: 1 Source/WebKit/qt/Api/qwebview.cpp:37: Alphabetical sorting problem. [build/include_order] [4] Source/WebKit/qt/Api/qwebview.cpp:38: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 91169 [details] Close Icon and Local Storage Databases on Quit Welcome to the Review-a-thon of the WebKit summit :) I'm afraid that this is not the correct solution. I would prefer if this would happen automatically when the last QWebPage instance is destructed. This could be done using a simple reference count for example.
Created attachment 91943 [details] Close Local Storage when last QWebPage is destroyed. Added an instance count to QWebPage. Now when the last QWebPage is destroyed, we close local storage and icon database as done on other platforms. This was clean up which did not change any functionality, so did not add any new tests.
Comment on attachment 91943 [details] Close Local Storage when last QWebPage is destroyed. View in context: https://bugs.webkit.org/attachment.cgi?id=91943&action=review > Source/WebKit/qt/Api/qwebpage.cpp:393 > + // if destroying last WebPage instance. Release resources. // If destroying last WebPage instance, release resources. > Source/WebKit/qt/Api/qwebpage.cpp:394 > + if (instanceCount <= 0) How can it be less than 0? > Source/WebKit/qt/Api/qwebpage_p.h:214 > + static int instanceCount; // Used to clean up when last instance destroyed. unneeded two spaces.
Created attachment 91961 [details] Make changes listed in Comment #7.
Comment on attachment 91961 [details] Make changes listed in Comment #7. View in context: https://bugs.webkit.org/attachment.cgi?id=91961&action=review Sorry , I missed a few comments. > Source/WebKit/qt/ChangeLog:12 > + Close Local Storage when last QWebPage is destroyed. > + > + Added an instance count to QWebPage. Now when the last QWebPage > + is destroyed, we close local storage and icon database as done > + on other platforms. Is this fixing any real world problem? a crash or what ever? can not this be tested? > Source/WebKit/qt/Api/qwebpage.cpp:170 > + WebCore::PageGroup::closeLocalStorage(); > +} > + > + Unneeded extra line. > Source/WebKit/qt/Api/qwebpage.cpp:393 > + // if destroying last WebPage instance. Release resources. remove the comment, it is clear from the code what is happening. > Source/WebKit/qt/Api/qwebpage.cpp:395 > + if (!instanceCount) > + shutDownWebKit(); I would add a assert before the 'if': ASSERT(instanceCount >= 0);
Comment on attachment 91961 [details] Make changes listed in Comment #7. View in context: https://bugs.webkit.org/attachment.cgi?id=91961&action=review > Source/WebKit/qt/ChangeLog:10 > + Added an instance count to QWebPage. Now when the last QWebPage Two spaces after "QWebPage." > Source/WebKit/qt/Api/qwebpage.cpp:166 > + WebCore::iconDatabase().close(); This should be inside #if ENABLE(ICONDATABASE) > Source/WebKit/qt/Api/qwebpage.cpp:167 > + WebCore::PageGroup::closeLocalStorage(); No need for the WebCore:: prefix here (or the line above.) > Source/WebKit/qt/Api/qwebpage.cpp:393 > + // if destroying last WebPage instance. Release resources. Comments should be in sentence style: // If destroying last QWebPage instance, release resources. > Source/WebKit/qt/Api/qwebpage_p.h:214 > + static int instanceCount; // Used to clean up when last instance destroyed. Unnecessary comment.
(In reply to comment #9) > (From update of attachment 91961 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=91961&action=review > Is this fixing any real world problem? a crash or what ever? can not this be tested? Very good question. Maybe I jumped the gun on fixing this one. I believe this error was originally reported as http://bugreports.qt.nokia.com/browse/QTWEBKIT-263. It indicates that closeLocalStorage is called on Mac and Windows, and probably should be called on other platforms too. It does not sound like a real world problem. The solutions pointed at on other platforms call both closeLocalStorage and IconDatabase::close(). Investigating on QtWebKit Linux, I do not see closeLocalStorage() do anything because the are no PageGroups created. Local Storage read/write in handled else where in WebCore::StorageAreaSync. And if I don't not call IconDatabase::close(), I do not see it getting called. I was thinking the most conservative approach was to make sure these were called on the QtWebKit platforms. Now I'm thinking that we should ignore it unless we have stronger evidence it is needed. Comments?
=== Bulk closing of Qt bugs === If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary. If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.