[webkit-reviews] review requested: [Bug 29425] [Qt] SIGSEGV after WebCore::Frame::loader() : [Attachment 41036] patch v3

[bugzilla-daemon at webkit.org]

jedrzej.nowacki at nokia.com has asked  for review:
Bug 29425: [Qt] SIGSEGV after WebCore::Frame::loader()
https://bugs.webkit.org/show_bug.cgi?id=29425

Attachment 41036: patch v3
https://bugs.webkit.org/attachment.cgi?id=41036&action=review

------- Additional Comments from jedrzej.nowacki at nokia.com
> In both cases you are stepping back through a deleted object? 
Yes, it is true (null pointer access).

> Have you considered doing the usual trick of having a 
> RefPtr<Frame> frame = m_originatingProgressFrame; in the method entry?

Yes, I have. Actually it was my first solution. But than I found an issue.
Should we call the didChangeEstimatedProgress hook if in the
postProgressEstimateChangedNotification developer try to stop loading? Moreover
the estimations are strange because loading is canceled (reset()). I found it a
bit confusing. But after a discussion in office with Jocelyn we decided that
the didChangeEstimatedProgress might be used as a cleanup function, so it is
better to call it. The new patch use RefPtr pattern.

> coding style violation, the c++ initializer list must be one variable per
> line.. and the naming of variables is also defined by the coding style.
Changed.

>> +	QTRY_VERIFY(tester.executed); // If fail it means that the test wasn't
executed.
> stupid question. how robust is that? should you wait for a loadFinished()
> signal or such?
QTRY_VERIFY could wait up to 5 seconds and the web pages are compiled into
resources ("qrc://..."). 
Even a slow machine should be fast enough :-). If you look into qtwebkit
autotests suite there is a lot of
similar solutions.