Bug 119654 - [Qt] Avoid calling QWebPage's virtual functions during deletion of QWebPage
Summary: [Qt] Avoid calling QWebPage's virtual functions during deletion of QWebPage
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Critical
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-08-10 07:36 PDT by Arunprasad Rajkumar
Modified: 2013-08-20 03:12 PDT (History)
6 users (show)

See Also:


Attachments
Patch (8.33 KB, patch)
2013-08-10 09:20 PDT, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff
Patch (12.42 KB, patch)
2013-08-12 15:05 PDT, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff
Patch (13.45 KB, patch)
2013-08-13 10:20 PDT, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff
Patch (14.28 KB, patch)
2013-08-15 11:21 PDT, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Arunprasad Rajkumar 2013-08-10 07:36:43 PDT
What is the problem ?
--------------------
class UnLoadTestPage : public QWebPage
{
    Q_OBJECT
public:
    virtual void javaScriptAlert(QWebFrame *originatingFrame, const QString& msg)
    {
        qDebug()<<"alert:"<<msg;
    }
};


UnLoadTestPage* tstPage = new UnLoadTestPage();
tstPage->mainFrame()->evaluateJavaScript("window.onunload=function(){alert('hello');}");
delete tstPage;

In the above sample code my overriden javaScriptAlert(..) won't be called, because onunload event is fired from ~QWebPage during that time overriden UnLoadTestPage is already destroyed.

What is the solution?
---------------------
1. Provide C++ API to send "unload" event(as like Blackberry port's WebPage::prepareToDestroy, http://trac.webkit.org/browser/trunk/Source/WebKit/blackberry/Api/WebPage.h#L114), so it is the responsibility of the QtWebKit embedder to call this new C++ API in their destructors(or before deleting their QWebPage instance).
(ex: tstPage->prepareToDestroy)

2. Provide C++ API to delete any QWebPage instance, which will take care of sending "unload" event before destroying any QWebPage instance.
(ex: tstPage->deletePage(); or QWebPage::deletePage(tstPage); //static C++ API)

3. Provide both #1 & #2.

Any Side effects?
-----------------
No.Existing flow is unaffected, if embedder didn't call QWebPage::prepareToDestroy then unload event will be generated as like the current flow.


I prefer solution #3 and will upload the fix soon.
Comment 1 Arunprasad Rajkumar 2013-08-10 09:20:01 PDT
Created attachment 208476 [details]
Patch
Comment 2 Allan Sandfeld Jensen 2013-08-10 15:36:31 PDT
Since QWebPage is already a wrapper around QWebPageAdaptor why can't you call QWebPageAdaptor::preparetoDestroy() from the qwebpage destructor?
Comment 3 Arunprasad Rajkumar 2013-08-11 00:02:02 PDT
(In reply to comment #2)
> Since QWebPage is already a wrapper around QWebPageAdaptor why can't you call QWebPageAdaptor::preparetoDestroy() from the qwebpage destructor?

When control reaches ~QWebPage derived class already been destroyed(~UnLoadTestPage).
Comment 4 Allan Sandfeld Jensen 2013-08-11 04:15:08 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > Since QWebPage is already a wrapper around QWebPageAdaptor why can't you call QWebPageAdaptor::preparetoDestroy() from the qwebpage destructor?
> 
> When control reaches ~QWebPage derived class already been destroyed(~UnLoadTestPage).

True, but that only matters if you need the overloaded alert like the test-case. Most common cases of what happens on unload would work, wouldn't it?
Comment 5 Allan Sandfeld Jensen 2013-08-11 04:20:29 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > Since QWebPage is already a wrapper around QWebPageAdaptor why can't you call QWebPageAdaptor::preparetoDestroy() from the qwebpage destructor?
> > 
> > When control reaches ~QWebPage derived class already been destroyed(~UnLoadTestPage).
> 
> True, but that only matters if you need the overloaded alert like the test-case. Most common cases of what happens on unload would work, wouldn't it?

Nevermind that was the entire case of the bug-report. 

If this depends on the user of qwebpage calling an extra method, could he call something to unload the page instead? 

Perhaps mainFram()->setHtml("") ?
Comment 6 Arunprasad Rajkumar 2013-08-11 06:44:54 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > Since QWebPage is already a wrapper around QWebPageAdaptor why can't you call QWebPageAdaptor::preparetoDestroy() from the qwebpage destructor?
> > > 
> > > When control reaches ~QWebPage derived class already been destroyed(~UnLoadTestPage).
> > 
> > True, but that only matters if you need the overloaded alert like the test-case. Most common cases of what happens on unload would work, wouldn't it?
> 
> Nevermind that was the entire case of the bug-report. 
> 
> If this depends on the user of qwebpage calling an extra method, could he call something to unload the page instead? 
> 
> Perhaps mainFram()->setHtml("") ?

Thanks Allan, I tried this, it is not working. Might be because of its async. nature of mainFrame()->setHtml() I guess :(
Comment 7 Allan Sandfeld Jensen 2013-08-12 03:16:05 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (In reply to comment #3)
> > > > (In reply to comment #2)
> > > > > Since QWebPage is already a wrapper around QWebPageAdaptor why can't you call QWebPageAdaptor::preparetoDestroy() from the qwebpage destructor?
> > > > 
> > > > When control reaches ~QWebPage derived class already been destroyed(~UnLoadTestPage).
> > > 
> > > True, but that only matters if you need the overloaded alert like the test-case. Most common cases of what happens on unload would work, wouldn't it?
> > 
> > Nevermind that was the entire case of the bug-report. 
> > 
> > If this depends on the user of qwebpage calling an extra method, could he call something to unload the page instead? 
> > 
> > Perhaps mainFram()->setHtml("") ?
> 
> Thanks Allan, I tried this, it is not working. Might be because of its async. nature of mainFrame()->setHtml() I guess :(

Another option might be to only introduce the close or unload method on qwebpage level and call QWebPageAdapter::deletePage(). I think it will do the same, but using the iterative detach/closeURL in FrameLoader.
Comment 8 Arunprasad Rajkumar 2013-08-12 10:59:39 PDT
Some review comments given by Allen(via IRC)

 - Implement the unload() method QWebFrame instead of QWebPage
 - Use detachFromParent()/cancelAndClear() of FrameLoader to send unload event instead of closeURL()
 - Finding appropriate place to call QWebFrame::unload implicitly without user intervention(may be side-effect of QWidget::close,..or similar)

Allan, Please correct me if I'm wrong :)
Comment 9 Arunprasad Rajkumar 2013-08-12 15:05:05 PDT
Created attachment 208566 [details]
Patch
Comment 10 Arunprasad Rajkumar 2013-08-12 22:00:01 PDT
(In reply to comment #9)
> Created an attachment (id=208566) [details]
> Patch

Allan, I feel calling unload implicitly from ~QWebViewPrivate & ~QGraphicsWebViewPrivate is more reasonable than closeEvent. What do you think?
Comment 11 Arunprasad Rajkumar 2013-08-13 10:20:46 PDT
Created attachment 208648 [details]
Patch
Comment 12 Arunprasad Rajkumar 2013-08-13 12:40:32 PDT
(In reply to comment #11)
> Created an attachment (id=208648) [details]
> Patch

Called unload() method implicitly from QWebViewPrivate::detachCurrentPage() & QGraphicsWebViewPrivate::detachCurrentPage().

So QtWebKit embedder uses QWebView/QGraphicsWebView, deletion of those will trigger unload() without any extra call.
Comment 13 Arunprasad Rajkumar 2013-08-15 11:21:28 PDT
Created attachment 208823 [details]
Patch
Comment 14 Arunprasad Rajkumar 2013-08-16 12:51:18 PDT
(In reply to comment #13)
> Created an attachment (id=208823) [details]
> Patch

As per your review comments added API test for QGraphicsWebView::close, QWebView::close and history count checking after unload.
Comment 15 Arunprasad Rajkumar 2013-08-19 12:26:17 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > Created an attachment (id=208823) [details] [details]
> > Patch
> 
> As per your review comments added API test for QGraphicsWebView::close, QWebView::close and history count checking after unload.

I tried with load(QUrl()), it worked. Thanks Allan & Jocelyn :)

I think using same fix http://trac.webkit.org/r61433 workaround can be removed. Isn't it?
Comment 16 Arunprasad Rajkumar 2013-08-19 23:55:52 PDT
Since calling mainFrame()->load(QUrl()) before deleting a custome WebPage object works as expected, I'm closing this bug.
Comment 17 Jocelyn Turcotte 2013-08-20 03:12:21 PDT
Comment on attachment 208823 [details]
Patch

Removing flags.