Bug 56375 – [Qt] PageGroup::closeLocalStorage() should be called when the app quits

• Component: Platform
• Status: RESOLVED INVALID

[Roger Scott]

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

[Roger Scott]

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

[Joe Wild]

Created attachment 90937 [details]
Small test to test local storage

[Joe Wild]

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.

[WebKit Review Bot]

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.

[Simon Hausmann]

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.

[Joe Wild]

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.

[Antonio Gomes]

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.

[Joe Wild]

Created attachment 91961 [details]
Make changes listed in Comment #7.

[Antonio Gomes]

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);

[Andreas Kling]

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.

[Joe Wild]

(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?

[Jocelyn Turcotte]

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