Bug 76700 – [Qt][WK2] Application URL schemes cause asserts when using debug.

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Zeno Albisser]

QtNetworkRequestData derives from RefCounted. Therefore it has to be used with RefPtr because otherwise ref() and deref() functions are not being called.
The purpose of QtNetworkRequestData and QtNetworkReplyData is to transfer network request/reply data over IPC. Therefore it must be allowed to construct / copy instances of these classes without using RefPtr.

Currently, whenever a QtNetworkRequestData object that was created on the stack runs out of scope, an ASSERT will trigger because ref()/deref() were never called.

[Zeno Albisser]

Created attachment 123297 [details]
patch for review.

[Kenneth Rohde Christiansen]

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

[Zeno Albisser]

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.

[Simon Hausmann]

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.

[Zeno Albisser]

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

[Zeno Albisser]

Created attachment 123545 [details]
patch for review. - updated as commented previously.

[Simon Hausmann]

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?

[Zeno Albisser]

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.

[Zeno Albisser]

Created attachment 123650 [details]
patch for review. - updated according to previous comments.

[Zeno Albisser]

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>

[Zeno Albisser]

All reviewed patches have been landed.  Closing bug.