| Summary: | [Qt][WK2] Application URL schemes cause asserts when using debug. | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Zeno Albisser <zeno> | ||||||||
| Component: | WebKit Qt | Assignee: | Zeno Albisser <zeno> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | cmarcelo, hausmann, jesus, kenneth, webkit.review.bot, zoltan | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Zeno Albisser
2012-01-20 04:51:52 PST
Created attachment 123297 [details]
patch for review.
Comment on attachment 123297 [details] patch for review. View in context: https://bugs.webkit.org/attachment.cgi?id=123297&action=review > Source/WebKit2/ChangeLog:16 > + Instances of QtNetworkRequestData / QtNetworkReplyData are meant > + to be used for transfering data over IPC. To allow transferring > + instances of these classes over IPC they need to be copyable, > + and it must be possible to create such instances on the stack. > + Because classes that inherit from RefCounted always need to be > + used in connection with RefPtr, QtNetworkRequestData and > + QtNetworkReplyData cannot inherit directly from RefCounted. > + > + Deleting an object that inherits from RefCounted, without > + the proper sequence of ref()/deref() being called by it's > + RefPtr, causes asserts when running a debug version. You dont explain why we need to ref count or why we cannot do it manually if needed. > Source/WebKit2/Shared/qt/QtNetworkRequestData.h:53 > +struct QtRefCountedNetworkRequestData : public WTF::RefCounted<QtRefCountedNetworkRequestData> { It seems that you want to use the ref counted data on each side but transfer only the data itself. Would a name like QtNetworkRequestDataHandle make more sense? The name here are a bit confusing.. we could also got for QtNetworkRequestData and then have a QtNetworkRequestDataImpl, Private, Internal or similar Comment on attachment 123297 [details] patch for review. View in context: https://bugs.webkit.org/attachment.cgi?id=123297&action=review >> Source/WebKit2/ChangeLog:16 >> + RefPtr, causes asserts when running a debug version. > > You dont explain why we need to ref count or why we cannot do it manually if needed. The IPC transfer infrastructure / messaging mostly consists of generated code. So there is no easy way to fix the refcounting manually in there. Further i would personally consider it pretty hackish to adjust ref counters manually. I think it would be pretty opposite to the expected behavior. >> Source/WebKit2/Shared/qt/QtNetworkRequestData.h:53 >> +struct QtRefCountedNetworkRequestData : public WTF::RefCounted<QtRefCountedNetworkRequestData> { > > It seems that you want to use the ref counted data on each side but transfer only the data itself. Would a name like > > QtNetworkRequestDataHandle make more sense? > > The name here are a bit confusing.. we could also got for QtNetworkRequestData and then have a QtNetworkRequestDataImpl, Private, Internal or similar My first local implementation was actually using QtNetworkRequestData/QtNetworkReqestDataImpl, but then i decided to rename these classes to QtRefCountedNetworkRequestData/QtNetworkRequestData. My impression is that QtNetworkRequestData is clearly describing this class of objects containing the data of a QtNetworkRequest. Appending Impl somehow sounded like a pleonasm to me. Further we would then also need to change the filenames to QtNetworkRequestDataImpl.h because the generated IPC code expects the class name of the transfered structure to match the filename. Then again it seems to me, that the name QtRefCountedNetworkRequestData describes its purpose very well. Comment on attachment 123297 [details] patch for review. View in context: https://bugs.webkit.org/attachment.cgi?id=123297&action=review Looks good in general, but I think a few fixes need to be applied to the refptr handling. See also http://www.webkit.org/coding/RefPtr.html. Setters that take ownership should take a PassRefPtr. Similarly for getters unless ownership is transferred the return type should be a raw pointer. > Source/WebKit2/UIProcess/API/qt/qquicknetworkrequest_p.h:38 > + void setNetworkRequestData(WTF::RefPtr<WebKit::QtRefCountedNetworkRequestData> data); Shouldn't this be a PassRefPtr? > Source/WebKit2/UIProcess/API/qt/qquicknetworkrequest_p.h:39 > + WTF::RefPtr<WebKit::QtRefCountedNetworkRequestData> networkRequestData() const; I believe this should return a raw pointer, as ownership is not transferred. Comment on attachment 123297 [details] patch for review. View in context: https://bugs.webkit.org/attachment.cgi?id=123297&action=review >> Source/WebKit2/UIProcess/API/qt/qquicknetworkrequest_p.h:38 >> + void setNetworkRequestData(WTF::RefPtr<WebKit::QtRefCountedNetworkRequestData> data); > > Shouldn't this be a PassRefPtr? Yes. - fixed. >> Source/WebKit2/UIProcess/API/qt/qquicknetworkrequest_p.h:39 >> + WTF::RefPtr<WebKit::QtRefCountedNetworkRequestData> networkRequestData() const; > > I believe this should return a raw pointer, as ownership is not transferred. I just figured out, i can actually remove this function. :) Created attachment 123545 [details]
patch for review. - updated as commented previously.
Comment on attachment 123545 [details] patch for review. - updated as commented previously. View in context: https://bugs.webkit.org/attachment.cgi?id=123545&action=review Sorry, not all RefPtrs are used correctly yet :) > Source/WebKit2/UIProcess/API/qt/qquicknetworkreply.cpp:125 > + uint64_t stringLength = m_dataLength / sizeof(UChar); > + return QString(reinterpret_cast<const QChar*>(m_sharedMemory->data()), stringLength); I see a wild mix of UChar and QChar here, assuming that they are of the same size. While that is indeed given, I think the code would be cleaner if it was using one type only. > Source/WebKit2/UIProcess/API/qt/qquicknetworkreply.cpp:137 > const UChar* ucharData = reinterpret_cast<const UChar*>(data.constData()); > uint64_t smLength = sizeof(UChar) * data.length(); Why bother with UChar at all? If the goal is to serialize the string, then why not simply copy the QChar data straight into it and also use sizeof(QChar)? > Source/WebKit2/UIProcess/API/qt/qquicknetworkreply_p.h:63 > + WTF::RefPtr<WebKit::QtRefCountedNetworkRequestData> networkRequestData() const; > + void setNetworkRequestData(WTF::RefPtr<WebKit::QtRefCountedNetworkRequestData> data); > + WTF::RefPtr<WebKit::QtRefCountedNetworkReplyData> networkReplyData() const; See my earlier comment, these functions should follow the refptr guidelines. > Source/WebKit2/UIProcess/API/qt/qquicknetworkrequest_p.h:33 > Q_PROPERTY(QString url READ url) Any particular reason the url is a string instead of a QUrl? Comment on attachment 123545 [details] patch for review. - updated as commented previously. View in context: https://bugs.webkit.org/attachment.cgi?id=123545&action=review >> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply.cpp:125 >> + return QString(reinterpret_cast<const QChar*>(m_sharedMemory->data()), stringLength); > > I see a wild mix of UChar and QChar here, assuming that they are of the same size. While that is indeed given, I think the code would be cleaner if it was using one type only. fixed. >> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply.cpp:137 >> uint64_t smLength = sizeof(UChar) * data.length(); > > Why bother with UChar at all? If the goal is to serialize the string, then why not simply copy the QChar data straight into it and also use sizeof(QChar)? replaced all UChar with QChar, simplified copying. >> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply_p.h:63 >> + WTF::RefPtr<WebKit::QtRefCountedNetworkReplyData> networkReplyData() const; > > See my earlier comment, these functions should follow the refptr guidelines. fixed. - returning raw pointers, using PassRefPtr for setter. >> Source/WebKit2/UIProcess/API/qt/qquicknetworkrequest_p.h:33 >> Q_PROPERTY(QString url READ url) > > Any particular reason the url is a string instead of a QUrl? changed this to use QUrl. Created attachment 123650 [details]
patch for review. - updated according to previous comments.
Comment on attachment 123650 [details] patch for review. - updated according to previous comments. Clearing flags on attachment: 123650 Committed r105711: <http://trac.webkit.org/changeset/105711> All reviewed patches have been landed. Closing bug. |