Bug 50267 – [Qt] Memory leaks for QWebPageClient

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Yi Shen]

Need to delete PageClientQWidget before creating a new one.

[Yi Shen]

Created attachment 75192 [details]
first try

[Yi Shen]

Created attachment 75229 [details]
remove unnecessary stuff

[Kenneth Rohde Christiansen]

Comment on attachment 75229 [details]
remove unnecessary stuff

Could it be an OwnPtr instead?

[WebKit Commit Bot]

Comment on attachment 75229 [details]
remove unnecessary stuff

Rejecting patch 75229 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 1
ERROR: Working directory has local commits, pass --force-clean to continue.

Full output: http://queues.webkit.org/results/6789010

[Yi Shen]

Comment on attachment 75229 [details]
remove unnecessary stuff

try it again

[WebKit Review Bot]

Comment on attachment 75229 [details]
remove unnecessary stuff

Rejecting patch 75229 from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Yi Shen]

(In reply to comment #3)
> (From update of attachment 75229 [details])
> Could it be an OwnPtr instead?

hmm, I will look at it later today. Thanks for the suggestion :)

[WebKit Commit Bot]

Comment on attachment 75229 [details]
remove unnecessary stuff

Rejecting patch 75229 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 1
ERROR: Working directory has local commits, pass --force-clean to continue.

Full output: http://queues.webkit.org/results/6793007

[Yi Shen]

Created attachment 75579 [details]
try again

Don't know why it was rejected by the commit bot, yael said it is a bug of commit bot, so just try it again. Thanks!

[Yi Shen]

(In reply to comment #3)
> (From update of attachment 75229 [details])
> Could it be an OwnPtr instead?
Tried OwnPtr but seems many places need to be changed...

[Eric Seidel (no email)]

Comment on attachment 75579 [details]
try again

Eek.  Manual delete for the loss!  Don't we have smart pointers we can use here?

[Yi Shen]

(In reply to comment #11)
> (From update of attachment 75579 [details])
> Eek.  Manual delete for the loss!  Don't we have smart pointers we can use here?
Qt Smart pointer like QPointer, doesn't work for QWebPageClient, since it is not derived from QObject.

[Kenneth Rohde Christiansen]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 75579 [details] [details])
> > Eek.  Manual delete for the loss!  Don't we have smart pointers we can use here?
> Qt Smart pointer like QPointer, doesn't work for QWebPageClient, since it is not derived from QObject.

You should just make the changes for it to be a OwnPtr or similar. It is not part of our public api, so you can easily do that.

[Yi Shen]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (From update of attachment 75579 [details] [details] [details])
> > > Eek.  Manual delete for the loss!  Don't we have smart pointers we can use here?
> > Qt Smart pointer like QPointer, doesn't work for QWebPageClient, since it is not derived from QObject.
> 
> You should just make the changes for it to be a OwnPtr or similar. It is not part of our public api, so you can easily do that.

OK, I will do it :)

[Yi Shen]

Created attachment 75821 [details]
use OwnPtr

[WebKit Review Bot]

Attachment 75821 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75821 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75821 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yi Shen]

Created attachment 75833 [details]
try again

Does buildbot hate my patch? :( I run the check-webkit-style on my machine and there is no error at all.

[WebKit Review Bot]

Attachment 75821 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yi Shen]

Created attachment 75835 [details]
remove unnecessary stuff

[WebKit Review Bot]

Attachment 75821 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061

Died at WebKitTools/Scripts/update-webkit line 132.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75833 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061

Died at WebKitTools/Scripts/update-webkit line 132.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75835 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061

Died at WebKitTools/Scripts/update-webkit line 132.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Comment on attachment 75835 [details]
remove unnecessary stuff

View in context: https://bugs.webkit.org/attachment.cgi?id=75835&action=review

> WebKit/qt/Api/qgraphicswebview.cpp:83
> +        return static_cast<PageClientQGraphicsWidget*>(page->d->client.get())->overlay;

Seems there should be a d->pageClient() accessor which does this cast for you, no?

> WebKit/qt/Api/qwebview.cpp:-355
> -    if (page->d->client && page->d->client->isQWidgetClient())

So the isQWidgetClient path was always taken?

> WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:-1538
> -            QWebPageClient* client = m_webFrame->page()->d->client;

The local was helpful to readabilty here.  A .get() would have worked fine.

[Yi Shen]

Created attachment 76240 [details]
updated with eric's suggestion

[Kenneth Rohde Christiansen]

Comment on attachment 76240 [details]
updated with eric's suggestion

View in context: https://bugs.webkit.org/attachment.cgi?id=76240&action=review

> WebKit/qt/Api/qwebpage.cpp:379
> +WebCore::PageClientQWidget* QWebPagePrivate::pageClientQWidget()

this is normally not how we do the cast wrappers. 

Somethink like

static PageClientQWidget* toPageClientQWidget(QWebPageClient* client)

you also want thwse to be inline

it just seems more appropriate adding a method like this to QWebViewPrivate. like

PageClientQWidget* QWebViewPrivate::pageClient()

[Yi Shen]

(In reply to comment #27)
> (From update of attachment 76240 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76240&action=review
> 
> > WebKit/qt/Api/qwebpage.cpp:379
> > +WebCore::PageClientQWidget* QWebPagePrivate::pageClientQWidget()
> 
> this is normally not how we do the cast wrappers. 
> 
> Somethink like
> 
> static PageClientQWidget* toPageClientQWidget(QWebPageClient* client)
> 
> you also want thwse to be inline
> 
> it just seems more appropriate adding a method like this to QWebViewPrivate. like
> 
> PageClientQWidget* QWebViewPrivate::pageClient()

Thanks Kenneth, I see what you mean. I will update patch today :)

[Yi Shen]

Created attachment 76310 [details]
try again

[Kenneth Rohde Christiansen]

Comment on attachment 76310 [details]
try again

View in context: https://bugs.webkit.org/attachment.cgi?id=76310&action=review

> WebKit/qt/Api/qwebpage_p.h:164
> +    OwnPtr<QWebPageClient> client;

we should rename QWebPageClient to just QtPageClient to not make it seem as a class defined in Qt while still showing that it is a qt port specific class

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 76310 [details]:

fast/preloader/script.html bug 50879 (author: [e-mail])
fast/loader/recursive-before-unload-crash.html bug 50880 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 76310 [details]
try again

Clearing flags on attachment: 76310

Committed r73867: <http://trac.webkit.org/changeset/73867>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.