Bug 29803

Summary:

[Qt] QWebHistory and QWebPage autotests crash

Product:

WebKit

Reporter:

Jędrzej Nowacki <jedrzej.nowacki>

Component:

WebKit Qt

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

eric, hausmann, jedrzej.nowacki, kenneth, zecke

Priority:

Keywords:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

29867

Attachments:

Description	Flags
create mainframe if needed	hausmann: review-
New patch	none
optimization	eric: review-
optimization+comment	hausmann: review-
patch	zecke: review-
patch next version	zecke: review-

Jędrzej Nowacki

Reported 2009-09-28 04:46:56 PDT

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

Attachments
create mainframe if needed (1.04 KB, patch) 2009-09-28 04:57 PDT, Jędrzej Nowacki	hausmann: review-	Details Formatted Diff Diff
New patch (3.44 KB, patch) 2009-09-29 03:59 PDT, Jędrzej Nowacki	no flags	Details Formatted Diff Diff
optimization (2.39 KB, patch) 2009-10-01 04:05 PDT, Jędrzej Nowacki	eric: review-	Details Formatted Diff Diff
optimization+comment (2.67 KB, patch) 2009-10-05 04:41 PDT, Jędrzej Nowacki	hausmann: review-	Details Formatted Diff Diff
patch (16.86 KB, patch) 2009-10-22 02:42 PDT, Jędrzej Nowacki	zecke: review-	Details Formatted Diff Diff
patch next version (17.42 KB, patch) 2009-10-22 08:53 PDT, Jędrzej Nowacki	zecke: review-	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Jędrzej Nowacki

Comment 1 2009-09-28 04:57:32 PDT

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 2 2009-09-28 05:28:13 PDT

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

Comment 3 2009-09-29 03:59:24 PDT

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

Comment 4 2009-09-29 04:00:40 PDT

Uncreated mainFrame influence QWebPage autotest too.

Simon Hausmann

Comment 5 2009-09-29 06:49:24 PDT

Comment on attachment 40293 [details] New patch r=me

Simon Hausmann

Comment 6 2009-09-29 06:50:27 PDT

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

Simon Hausmann

Comment 7 2009-09-29 07:07:28 PDT

Sorry, wrong changeset. The fix was landed in 48871

Kenneth Rohde Christiansen

Comment 8 2009-09-29 17:57:54 PDT

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

Comment 9 2009-09-30 00:47:25 PDT

(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

Comment 10 2009-09-30 05:31:41 PDT

(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

Comment 11 2009-09-30 06:58:19 PDT

> 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

Comment 12 2009-09-30 07:34:12 PDT

(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

Comment 13 2009-10-01 04:05:32 PDT

Created attachment 40432 [details] optimization add some checks to QWebPage and Page to avoid mainframe construction in QWbPage destructor

Eric Seidel (no email)

Comment 14 2009-10-02 12:37:33 PDT

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

Comment 15 2009-10-05 04:41:40 PDT

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)

Comment 16 2009-10-05 09:30:08 PDT

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 17 2009-10-07 07:52:53 PDT

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 18 2009-10-13 07:10:19 PDT

Comment on attachment 40293 [details] New patch Clearing review flag as this patch has been landed.

Jędrzej Nowacki

Comment 19 2009-10-22 02:42:55 PDT

Created attachment 41649 [details] patch

Holger Freyther

Comment 20 2009-10-22 04:51:58 PDT

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

Comment 21 2009-10-22 08:53:24 PDT

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

Comment 22 2009-10-22 09:00:06 PDT

(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

Comment 23 2009-10-23 20:52:47 PDT

(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 24 2009-10-23 21:09:12 PDT

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

Comment 25 2009-10-26 07:26:28 PDT

> > #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 26 2009-10-26 16:09:41 PDT

Comment on attachment 41660 [details] patch next version Clearing cq? on this r-'d patch.

Holger Freyther

Comment 27 2009-10-26 20:06:11 PDT

(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

Comment 28 2009-10-27 01:33:04 PDT

(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

Comment 29 2009-11-11 02:12:30 PST

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.

Note You need to log in before you can comment on or make changes to this bug.