Bug 29803 – [Qt] QWebHistory and QWebPage autotests crash

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Jędrzej Nowacki]

Autotest saveAndRestore_crash_2 crashed because WebCore::Page::m_mainFrame was uninitialized. It means that mainFrame hadn't been created before Page::goToItem was called.

backtrace:
#0  0xb75b1e6a in WebCore::FrameLoader::stopAllLoaders(WebCore::DatabasePolicy) () from /home/nierob/dev/webkit/WebKitBuild/Release/lib/libQtWebKit.so.4
#1  0xb761e028 in WebCore::Page::goToItem(WebCore::HistoryItem*, WebCore::FrameLoadType) () from /home/nierob/dev/webkit/WebKitBuild/Release/lib/libQtWebKit.so.4
#2  0xb77bef00 in QWebHistory::goToItem(QWebHistoryItem const&) () from /home/nierob/dev/webkit/WebKitBuild/Release/lib/libQtWebKit.so.4
#3  0xb77bf0c5 in QWebHistory::restoreState(QByteArray const&) () from /home/nierob/dev/webkit/WebKitBuild/Release/lib/libQtWebKit.so.4
#4  0x0804ad8e in tst_QWebHistory::saveAndRestore_crash_2() ()
#5  0x0804df6a in tst_QWebHistory::qt_metacall(QMetaObject::Call, int, void**) ()
#6  0xb613bb8f in QMetaObject::metacall (object=0xbffff240, cl=QMetaObject::InvokeMetaMethod, idx=20, argv=0xbfffe5f0) at /home/nierob/dev/qt/src/corelib/kernel/qmetaobject.cpp:237
#7  0xb613f377 in QMetaMethod::invoke (this=0xbfffe974, object=0xbffff240, connectionType=Qt::DirectConnection, returnValue=..., val0=..., val1=..., val2=..., val3=..., val4=..., val5=..., val6=..., val7=..., val8=..., val9=...)
    at /home/nierob/dev/qt/src/corelib/kernel/qmetaobject.cpp:1533
#8  0xb613fb59 in QMetaObject::invokeMethod (obj=0xbffff240, member=0x8158da8 "saveAndRestore_crash_2", type=Qt::DirectConnection, ret=..., val0=..., val1=..., val2=..., val3=..., val4=..., val5=..., val6=..., val7=..., val8=..., val9=...)
    at /home/nierob/dev/qt/src/corelib/kernel/qmetaobject.cpp:1113
#9  0xb706d776 in QMetaObject::invokeMethod (obj=0xbffff240, member=0x8158da8 "saveAndRestore_crash_2", type=Qt::DirectConnection, val0=..., val1=..., val2=..., val3=..., val4=..., val5=..., val6=..., val7=..., val8=..., val9=...)
    at ../../include/QtCore/../../src/corelib/kernel/qobjectdefs.h:396
#10 0xb7067f78 in qInvokeTestMethodDataEntry (slot=0x8158da8 "saveAndRestore_crash_2") at qtestcase.cpp:1212
#11 0xb7069452 in qInvokeTestMethod (slotName=0x804f29b "saveAndRestore_crash_2()", data=0x0) at qtestcase.cpp:1307
#12 0xb7069c6e in qInvokeTestMethods (testObject=0xbffff240) at qtestcase.cpp:1462
#13 0xb706acc3 in QTest::qExec (testObject=0xbffff240, argc=1, argv=0xbffff314) at qtestcase.cpp:1661
#14 0x0804a949 in main ()

[Jędrzej Nowacki]

Created attachment 40227 [details]
create mainframe if needed

I'm not sure if it is the best solution. m_mainFrame can't be created in QWebPage constructor (it should be a way to connect a frameCreated signal). 
In the patch, QWebPage::history() and QWebPage::currentFrame() call QWebPagePrivate::createMainFrame() as a side effect. It is rather bad, why history need a frame to work? Maybe it is better to modify WebCore::Page class instead?

[Simon Hausmann]

Comment on attachment 40227 [details]
create mainframe if needed

Looks good to me, but there's a ChangeLog entry missing and the position of the '*' next to FrameLoader* loader is wrong.

Can you fix these and re-upload? Thanks!

[Jędrzej Nowacki]

Created attachment 40293 [details]
New patch

Crashes are not only in QWebHistory but also in QWebPage. The patch cover most of them (all produced by uncreated mainFrame). Autotest was added.

[Jędrzej Nowacki]

Uncreated mainFrame influence QWebPage autotest too.

[Simon Hausmann]

Comment on attachment 40293 [details]
New patch

r=me

[Simon Hausmann]

Committed r48869: <http://trac.webkit.org/changeset/48869>

[Simon Hausmann]

Sorry, wrong changeset. The fix was landed in 48871

[Kenneth Rohde Christiansen]

 QWebPage::~QWebPage()
 {
+    d->createMainFrame();

Why does it make sense to create a main frame in the destructor? Shouldn't there instead be a test to see if it was not created in the following code?

At least I think it deserves a comment.

[Jędrzej Nowacki]

(In reply to comment #8)
>  QWebPage::~QWebPage()
>  {
> +    d->createMainFrame();
> 
> Why does it make sense to create a main frame in the destructor? Shouldn't
> there instead be a test to see if it was not created in the following code?
> 
> At least I think it deserves a comment.

Actually d->createMainFrame() is a test, it test if mainFrame was created. MainFrame should be created as soon as possible, but not in constructor because of signals. WebCore::Page really depends on mainFrame and use it in different places (in destructor too). 

Maybe we should move all checks to WebCore::Page, but:
- the change will affect more platforms, not only Qt
- it will be much more difficult to control mainFrame creation time. If Page automagically will create frame how to emit signal?
In general it is possible but what we can gain with it?

[Simon Hausmann]

(In reply to comment #9)
> (In reply to comment #8)
> >  QWebPage::~QWebPage()
> >  {
> > +    d->createMainFrame();
> > 
> > Why does it make sense to create a main frame in the destructor? Shouldn't
> > there instead be a test to see if it was not created in the following code?
> > 
> > At least I think it deserves a comment.
> 
> Actually d->createMainFrame() is a test, it test if mainFrame was created.
> MainFrame should be created as soon as possible, but not in constructor because
> of signals. WebCore::Page really depends on mainFrame and use it in different
> places (in destructor too). 
> 
> Maybe we should move all checks to WebCore::Page, but:
> - the change will affect more platforms, not only Qt
> - it will be much more difficult to control mainFrame creation time. If Page
> automagically will create frame how to emit signal?
> In general it is possible but what we can gain with it?

I think we should remove the creation from the QWebPage destructor and add just that one single if in the Page destructor.

[Jędrzej Nowacki]

> I think we should remove the creation from the QWebPage destructor and add just
> that one single if in the Page destructor.

I don't like this idea. It seems to be only partial solution which could bit as later. 

I will prepare prototype of WebCore::Page with checking for mainFrame - just crash blockers. We will see how it looks like :-)

[Jędrzej Nowacki]

(In reply to comment #11)
> > I think we should remove the creation from the QWebPage destructor and add just
> > that one single if in the Page destructor.
> 
> I don't like this idea. It seems to be only partial solution which could bit as
> later. 
> 
> I will prepare prototype of WebCore::Page with checking for mainFrame - just
> crash blockers. We will see how it looks like :-)

No, that was bad idea too. There is problem, when actually create frame? When it is needed and when it is only a strange function call? There is no function called creatMainFrame in public API. So developer can't actually create the frame manually. 

Aghr, it looks like design issue. It is a bit similar to our discussion about QWebInspector...

Ok, I'll prepare patch only for ~Page...

[Jędrzej Nowacki]

Created attachment 40432 [details]
optimization

add some checks to QWebPage and Page to avoid mainframe construction in QWbPage destructor

[Eric Seidel (no email)]

Comment on attachment 40432 [details]
optimization

Why:
     m_mainFrame->setView(0);
 172     if (m_mainFrame)
 173         m_mainFrame->setView(0);
That needs to be explained in the ChangeLog and possibly a comment.

Also, this needs a test.

[Jędrzej Nowacki]

Created attachment 40624 [details]
optimization+comment

Comment was added. This mean that we force comments in source code? Cool, I would love to see more comments in webkit :-)
Changelog was changed. I hope it is better.
Autotest is not needed. This case is covered by tst_QWebPage::crashTests_LazyInitializationOfMainFrame()

[Eric Seidel (no email)]

There is a patch posted for review on this closed bug.  Should be the bug be re-opened or the review flags cleared?  It looks from the comments like this should be re-opened.  Opening...

[Simon Hausmann]

Comment on attachment 40624 [details]
optimization+comment

Jedrzej is looking into a different solution right now, that will hopefully allow us to remove the lazy initialization of the main frame (as per WebCore::Page contract) but still preserve the createFramed() signal behaviour.

[Simon Hausmann]

Comment on attachment 40293 [details]
New patch

Clearing review flag as this patch has been landed.

[Jędrzej Nowacki]

Created attachment 41649 [details]
patch

[Holger Freyther]

Comment on attachment 41649 [details]
patch


>      frameLoaderClient->setFrame(qframe, frame);
> -
>      frame->init();
> +    // Delay the signal QWebPage::frameCreated(QWebFrame*), because the QwebPage might be not
> +    // fully created (no way to connect to the signal).

Whitespace, spelling... QwebPage vs. QWebPage..



> +        Emission of the frameCreated signal was moved to the QWebFrame::init()
> +        and delayed.

I didn't read most of the bug report. The ChangeLog should answer the question why? The QWebPage pretty much knows when the mainFrame got created? Why do we need to delay this signal?

[Jędrzej Nowacki]

Created attachment 41660 [details]
patch next version

Patch's changelog :-)
- I corrected spelling.
- Changelog is even more verbose now.
- Coding style:
$ WebKitTools/Scripts/check-webkit-style --git-commit=HEAD~1
Done processing WebKit/qt/tests/qwebpage/tst_qwebpage.cpp
Done processing WebKit/qt/Api/qwebframe.cpp
Done processing WebKit/qt/Api/qwebpage.cpp
Done processing WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp
Done processing WebKit/qt/Api/qwebframe_p.h
Done processing WebKit/qt/Api/qwebpage.h
Done processing WebKit/qt/Api/qwebframe.h
Total errors found: 0

[Jędrzej Nowacki]

(In reply to comment #20)
> I didn't read most of the bug report. 
Why? How can you review the patch if you don't know the background?

Thanks for feedback even if it was "ugly" r- :-).

[Holger Freyther]

(In reply to comment #22)
> (In reply to comment #20)
> > I didn't read most of the bug report. 
> Why? How can you review the patch if you don't know the background?
> 
> Thanks for feedback even if it was "ugly" r- :-).

I don't think this comment is appropriate. Enforcing the coding style is one of the tasks of reviewers and you failed repetedly on it. In general we tend to get the form right before looking into content. It is just how it is...

[Holger Freyther]

Comment on attachment 41660 [details]
patch next version

> diff --git a/WebKit/qt/Api/qwebframe.cpp b/WebKit/qt/Api/qwebframe.cpp
> index 45dbfba..d5b5f9e 100644
> --- a/WebKit/qt/Api/qwebframe.cpp
> +++ b/WebKit/qt/Api/qwebframe.cpp
> @@ -41,6 +41,7 @@
>  #include "JSDOMWindowBase.h"
>  #include "JSLock.h"
>  #include "JSObject.h"
> +#include "moc_qwebframe.cpp"


No... can you show me any other file doing that? We tend to put it at the bottom...below anything else..


>  #include "NodeList.h"
>  #include "Page.h"
>  #include "PlatformMouseEvent.h"
> @@ -207,15 +208,21 @@ QWebFrameData::QWebFrameData(WebCore::Page* parentPage, WebCore::Frame* parentFr
>  void QWebFramePrivate::init(QWebFrame *qframe, QWebFrameData *frameData)
>  {
>      q = qframe;
> -

still a bogus whitespace change...



>      static WebCore::Frame* core(QWebFrame*);
>      static QWebFrame* kit(WebCore::Frame*);
> +    static QWebFrame* get(QWebFramePrivate* priv) { return priv->q; }
> +    static QWebFramePrivate* get(QWebFrame* pub) { return pub->d; }
>  

they are not used, there is no reason to have this in the patch.


> --- a/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp
> +++ b/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp
> @@ -123,11 +123,14 @@ private slots:
>      void inputMethods();
>      void defaultTextEncoding();
>      void errorPageExtension();
> -
>      void crashTests_LazyInitializationOfMainFrame();
> -

why did you remove this space?


> +void tst_QWebPage::frameCreatedSignal()
> +{
> +    // A MainFrame should be created before ~QWebPage because the WebCore::Page (the base classs)
> +    // use the frame in the destructor.
> +    QWebPage page;
> +    bool sign = waitForSignal(&page, SIGNAL(frameCreated(QWebFrame*)));
> +    QVERIFY2(sign, "A QWebFrame hasn't been created before ~QWebPage");
> +}
> +

One thing I don't like about this is the following.

if I change the test case above to do

QWebPage page;
page.load("<script>window.print()</script>");
bool sign = waitForSi...

I will be likely to get the "printRequested(QWebFrame*)" before the frameCreated signal... which is making things worse...

I think the API contract should be changed to not emit frameCreated from the QWebPage c'tor and that it is guranteed that the mainFrame() exists...

[Jędrzej Nowacki]

> >  #include "NodeList.h"
> >  #include "Page.h"
> >  #include "PlatformMouseEvent.h"
> > @@ -207,15 +208,21 @@ QWebFrameData::QWebFrameData(WebCore::Page* parentPage, WebCore::Frame* parentFr
> >  void QWebFramePrivate::init(QWebFrame *qframe, QWebFrameData *frameData)
> >  {
> >      q = qframe;
> > -
>
> still a bogus whitespace change...
[snap & add]
>
> > --- a/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp
> > +++ b/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp
> > @@ -123,11 +123,14 @@ private slots:
> >      void inputMethods();
> >      void defaultTextEncoding();
> >      void errorPageExtension();
> > -
> >      void crashTests_LazyInitializationOfMainFrame();
> > -
>
> why did you remove this space?
<BadJoke> I removed these lines to speed up compilation </BadJoke>
I've just removed an empty line... Why it is bad? Why the line should be there? Why you say it is a bogus change?
Really, the lines are meaningless and actually I think they are confusing, an empty lines should divide some logical blocks, but this isn't the case. Since I had to modify the part of source code I decided to reformat it.

>
>
> >      static WebCore::Frame* core(QWebFrame*);
> >      static QWebFrame* kit(WebCore::Frame*);
> > +    static QWebFrame* get(QWebFramePrivate* priv) { return priv->q; }
> > +    static QWebFramePrivate* get(QWebFrame* pub) { return pub->d; }
> >
>
> they are not used, there is no reason to have this in the patch.
These two standard function are harmless, I missed them in a point of time :-). I agree it is irrelevant for the patch.

> One thing I don't like about this is the following.
>
> if I change the test case above to do
>
> QWebPage page;
> page.load("<script>window.print()</script>");
> bool sign = waitForSi...
>
> I will be likely to get the "printRequested(QWebFrame*)" before the
> frameCreated signal... which is making things worse...
>
> I think the API contract should be changed to not emit frameCreated from the
> QWebPage c'tor and that it is guranteed that the mainFrame() exists...
This is an awesome catch! Thanks. Because of that the solution is completely bad. The only workarounds I can find for the problem are based on MagicSignalRouter(tm) class or on some operation on the EventLoop. This would be evil and an overkill.
So I believe we should leave current solution. It is more readable and it will be easier to maintenance.

[Eric Seidel (no email)]

Comment on attachment 41660 [details]
patch next version

Clearing cq? on this r-'d patch.

[Holger Freyther]

(In reply to comment #25)

(reordered for importance)

> > I think the API contract should be changed to not emit frameCreated from the
> > QWebPage c'tor and that it is guranteed that the mainFrame() exists...
> This is an awesome catch! Thanks. Because of that the solution is completely
> bad. The only workarounds I can find for the problem are based on
> MagicSignalRouter(tm) class or on some operation on the EventLoop. This would
> be evil and an overkill.
> So I believe we should leave current solution. It is more readable and it will
> be easier to maintenance.

hehe, I like the killing of createMainFrame in your solution a lot. I will try to poke Simon once he is back from vacation.


> > why did you remove this space?
> <BadJoke> I removed these lines to speed up compilation </BadJoke>
> I've just removed an empty line... Why it is bad? Why the line should be there?
> Why you say it is a bogus change?
> Really, the lines are meaningless and actually I think they are confusing, an
> empty lines should divide some logical blocks, but this isn't the case. Since I
> had to modify the part of source code I decided to reformat it.

Right they are meaningless, why do you change them then? The main point is that you propose to fix a specific problem and when the patch contains unrelated things it gets harder to review. As a reviewer you will have to ask yourself if the contributor is using a Yedi mind trick to fool you or what he tries to hide.

The other part is that if one is not enforcing a conherent coding style one ends up having a mess like the symbian kernel where {} are freely placed with every kind of indention.

[Jędrzej Nowacki]

(In reply to comment #27)
> (In reply to comment #25)
> > > I think the API contract should be changed to not emit frameCreated from the
> > > QWebPage c'tor and that it is guranteed that the mainFrame() exists...
> > This is an awesome catch! Thanks. Because of that the solution is completely
> > bad. The only workarounds I can find for the problem are based on
> > MagicSignalRouter(tm) class or on some operation on the EventLoop. This would
> > be evil and an overkill.
> > So I believe we should leave current solution. It is more readable and it will
> > be easier to maintenance.
>
> hehe, I like the killing of createMainFrame in your solution a lot. I will try
> to poke Simon once he is back from vacation.
Another solution: Add info to documentation that QWebPage constructor always creates the main frame and it is not possible to catch the signal. It isn't big deal because of the QWebPage::mainFrame method. The solution have one disadvantage it can be implemented only in a major release of Qt, it is behavior change. Maybe it is worth to wait for it. Current solution is not so bad.

> > Really, the lines are meaningless and actually I think they are confusing, an
> > empty lines should divide some logical blocks, but this isn't the case. Since I
> > had to modify the part of source code I decided to reformat it.
>
> Right they are meaningless, why do you change them then? The main point is that
> you propose to fix a specific problem and when the patch contains unrelated
> things it gets harder to review. As a reviewer you will have to ask yourself if
> the contributor is using a Yedi mind trick to fool you or what he tries to
> hide.
OK, I understand now, I'll try to avoid it.

> The other part is that if one is not enforcing a conherent coding style one
> ends up having a mess like the symbian kernel where {} are freely placed with
> every kind of indention.
Agree, but I don't think it should be done by humans, we have computers and cool scripts for it. OK, I know it isn't the place and the time for this discussion.

[Jędrzej Nowacki]

I talked with Simon and we decided to leave current solution as the easiest one.

I'm closing bug as "resolved fixed" because patch is committed and the autotest doesn't crash anymore.