[Qt] Avoid calling QWebPage's virtual functions during deletion of QWebPage

• Component: WebKit Qt
• Status: RESOLVED INVALID

[Arunprasad Rajkumar]

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.

[Arunprasad Rajkumar]

Created attachment 208476 [details]
Patch

[Allan Sandfeld Jensen]

Since QWebPage is already a wrapper around QWebPageAdaptor why can't you call QWebPageAdaptor::preparetoDestroy() from the qwebpage destructor?

[Arunprasad Rajkumar]

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

[Allan Sandfeld Jensen]

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

[Allan Sandfeld Jensen]

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

[Arunprasad Rajkumar]

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

[Allan Sandfeld Jensen]

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

[Arunprasad Rajkumar]

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

[Arunprasad Rajkumar]

Created attachment 208566 [details]
Patch

[Arunprasad Rajkumar]

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

[Arunprasad Rajkumar]

Created attachment 208648 [details]
Patch

[Arunprasad Rajkumar]

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

[Arunprasad Rajkumar]

Created attachment 208823 [details]
Patch

[Arunprasad Rajkumar]

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

[Arunprasad Rajkumar]

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

[Arunprasad Rajkumar]

Since calling mainFrame()->load(QUrl()) before deleting a custome WebPage object works as expected, I'm closing this bug.

[Jocelyn Turcotte]

Comment on attachment 208823 [details]
Patch

Removing flags.