Resolve relative URLs within RSS article description #21943

zent1n0 · 2024-12-03T07:26:32Z

In RSS widget, a relative url will cause an infinite loop loading for resource, which won't break until unselect the article explicitly, thus caused the memory leak described in #20117 .

The code provides a conversion over the first 3 forms of relative path as described in MDN web docs using regex, and only function on <a href> and <img src> html tags. With the code, RSS widget now should follow the correct absolute urls.

However, the #20117 could not be fully closed with the code, as now log shows the RSS widget still send around 5 reqs/sec in the condition which was causing a rapid flow of qDebug log which indicates it stuck on a infinite loop sending requests, failing fast and immediately retries. The former situation is eating up a single CPU core at 100% and 3 MiB/s RAM increase, while the latter at below 10% CPU usage and 1 MiB/s RAM.

From the snapshot you could know the url is correct, but with no valid response (QNetworkReply::RemoteHostClosedError) and I don't know why. The fact is that the site byr.pt is an IPv6 only site and the url will provide an 30x jump to another path.

Happy with any assistance or suggestions on further improvements. 😃 🚀

glassez

Only some preliminary comments.

glassez · 2024-12-05T11:19:21Z

.gitignore

@@ -1,4 +1,5 @@
 .vscode/
+.cache


If you believe that qBittorrent .gitignore should contain it you need to provide separate PR with description of why it is needed to be done at project level. Otherwise you could just add it in your local .gitignore file.

Thanks, I'm using clangd extension and it produces many file under .cache/. I'll draft another PR later.

glassez · 2024-12-05T11:23:34Z

src/gui/rss/rsswidget.cpp

+    QString normalizeBasePath = basePath.endsWith(u'/') ? basePath : basePath + u'/';
+    QRegularExpressionMatchIterator iter = rx.globalMatch(html);
+
+    while (iter.hasNext()) {


Invalid coding style.

Suggested change

while (iter.hasNext()) {

while (iter.hasNext())

{

glassez · 2024-12-05T11:26:19Z

src/gui/rss/rsswidget.cpp

+    rx.setPattern(
+        uR"(((<a\s+[^>]*?href|<img\s+[^>]*?src)\s*=\s*["'])((https?|ftp):)?(\/\/[^\/]*)?(\/?[^\/"].*?)(["']))"_s);
+
+    QString normalizeBasePath = basePath.endsWith(u'/') ? basePath : basePath + u'/';


Suggested change

QString normalizeBasePath = basePath.endsWith(u'/') ? basePath : basePath + u'/';

QString normalizedBasePath = basePath.endsWith(u'/') ? basePath : basePath + u'/';

glassez · 2024-12-05T11:31:35Z

src/gui/rss/rsswidget.cpp

+        QString relativePath = match.captured(6);
+        if (relativePath.startsWith(u'/'))
+            relativePath = relativePath.mid(1);
+        if (!match.captured(4).isEmpty() && !match.captured(5).isEmpty())


It's hard to review/maintain regex-related logic so it would be better to either use named capturing groups or just assign them to local (const) variables before using in the code.

In RSS widget, a relative url will cause an infinite loop loading for resource, which won't break until unselect the article explicitly format code

zent1n0 · 2024-12-05T14:14:04Z

Fixed a bug with //{hostname} like URL and some coding styles, should make the regex logic more friendly to read.

glassez

Yet another review iteration.

src/gui/rss/rsswidget.cpp

glassez · 2024-12-05T14:23:28Z

src/gui/rss/rsswidget.h

@@ -91,6 +91,7 @@ private slots:

 private:
    bool eventFilter(QObject *obj, QEvent *event) override;
+    void convertRelativePathToAbsolute(QString &html, const QString &basePath) const;


This function does not need (and isn't supposed to need) access to RSSWidget class members, so it is preferable to declare it in anonymous namespace in rsswidget.cpp file.

src/gui/rss/rsswidget.cpp

Co-authored-by: Vladimir Golovnev <[email protected]>

zent1n0 · 2024-12-06T03:27:32Z

Adjusted my code. Could you review the code for this round @glassez

Chocobo1

Only commenting on coding style.
I'll leave the correctness to others.

src/gui/rss/rsswidget.cpp

glassez · 2024-12-06T12:53:42Z

Convert relative paths to absolute

Well, using the wrong term was what looked confusing to me.
Don't we really talk about the URLs, not the paths?

Co-authored-by: Chocobo1 <[email protected]> Co-authored-by: Vladimir Golovnev <[email protected]>

glassez · 2024-12-06T17:11:00Z

src/gui/rss/rsswidget.cpp

@@ -54,6 +54,8 @@
 #include "feedlistwidget.h"
 #include "ui_rsswidget.h"

+void convertRelativeUrlToAbsolute(QString &html, const QString &baseUrl);


It should be inside anonymous namespace as I said before, i.e.:

namespace { void convertRelativeUrlToAbsolute(QString &html, const QString &baseUrl) { // code } }

My misunderstanding of anonymous namespace. I thought it meant an static scope in the source file. I'll correct it later.

glassez · 2024-12-06T17:18:51Z

src/gui/rss/rsswidget.cpp

+        const QString fullMatch = match.captured(0);
+        const QString prefix = match.captured(1);
+        const QString suffix = match.captured(7);
+        const QString absolutePath = normalizedBaseUrl + relativePath;


absoluteURL ???

True. Corrected. 🚀

src/gui/rss/rsswidget.cpp

zent1n0 · 2024-12-07T17:01:12Z

BTW it's not an final solution. The true issue is the RSS htmlbrowser infinitely retries immediate loading resources from html. The issue behind is hard to track with call stack, since it is called by signals and slots.

zent1n0 · 2024-12-11T16:50:53Z

@glassez Shall we start next review iteration? I'm looking forward to a release with less memory issue in RSS widget. 🚀

glassez · 2024-12-11T16:56:23Z

The true issue is the RSS htmlbroser infinitely retries immediate loading resources from html.

What do you mean? Does it repeatedly download the same images etc. from the Internet?

zent1n0 · 2024-12-12T15:22:47Z

Does it repeatedly download the same images etc. from the Internet?

Yes, and specially when the resource is not available.

In my case a relative URL /oss/userfiles/5e2d209f-2306-46b5-b596-e89cf1c57598 will be completed as http:///oss/userfiles/5e2d209f-2306-46b5-b596-e89cf1c57598 and fails in a millisecond. With my patch it converts into https://byr.pt/oss/userfiles/5e2d209f-2306-46b5-b596-e89cf1c57598 but still got a infinite loop at a slower pace. Maybe because the URL above leads to an redirect into https://oss.byr.pt/byrbt/userfiles/cb/cb785da349087f79d06183902f50b0d252fac050?{some_other_query_params}.

I'm wondering if the htmlbrowser calls to QNetworkRequest could perform with an option that allows redirect, and could get it working with RSS widget.

glassez · 2024-12-14T09:19:31Z

Does it repeatedly download the same images etc. from the Internet?

Yes, and specially when the resource is not available.

Yet another question. It behaves the same way if HTML initially contains absolute URLs, right?

zent1n0 · 2024-12-14T09:47:11Z

Does it repeatedly download the same images etc. from the Internet?

Yes, and specially when the resource is not available.

Yet another question. It behaves the same way if HTML initially contains absolute URLs, right?

No, the absolute URLs will be rendered normally most of the time. Sometimes, the failed elements display as a blank page icon, but I'm not cofident to tell if in this situation the infinite loading issue is occurring.

glassez · 2024-12-14T11:11:07Z

Yet another question. It behaves the same way if HTML initially contains absolute URLs, right?

No, the absolute URLs will be rendered normally most of the time. Sometimes, the failed elements display as a blank page icon, but I'm not cofident to tell if in this situation the infinite loading issue is occurring.

I'm confused. Do you still want to say that initially absolute URLs and URLs that was relative and transformed to absolute by your code behave differently? (provided that they point to existing resources)

zent1n0 · 2024-12-14T11:41:27Z

Yet another question. It behaves the same way if HTML initially contains absolute URLs, right?

Let's figure it out.

My convert function only rewrite the URL when the matched result is not absolute one. If it is absolute, as the line 641-647, this iteration should be skipped, left text unmodified. So the code will not perform differently with an initially absolute URL.
If you mean the issue itself, the initially absolue URLs sometimes fail to load.

Does it repeatedly download the same images etc. from the Internet?

No, the absolute URLs will be rendered normally most of the time. Sometimes, the failed elements display as a blank page icon, but I'm not cofident to tell if in this situation the infinite loading issue is occurring.

I mean I can't confirm the failed elements with an absolute URL would cause the same infinite loading loop, just the same behavior that is to stuck like relative ones mentioned in the issue.

Sorry for my poor English that might make the answer confusing 😢

glassez · 2024-12-16T08:51:50Z

If you mean the issue itself

Sure. I understand the logic of the code. I am interested in the results of your investigation on the problem itself.

Does it repeatedly download the same images etc. from the Internet?

Yes, and specially when the resource is not available.

The most important thing that interests me is whether it "repeatedly download the same images etc." when the resource IS vavailable and was successfully downloaded previously.

As for "not available" resources, we have either endless attempts to get them from a local disk (without this patch) or from a network location (with this patch).

Considering that this patch fixes the problem in the case where the converted to absolute URL points to an available resource, I approve it. The problem of endless redownload of non available resources seems to be on Qt side (however, it would be nice to inform them about it, so that they can fix it further).

src/gui/rss/rsswidget.cpp

Co-authored-by: Vladimir Golovnev <[email protected]>

zent1n0 · 2024-12-16T14:25:39Z

Thanks for your reviews and suggestions. Although it's not an final solution, I'm looking forward to these snippets in released version. I'll be grateful if you could give a probable prediction that which tag will this be included in.

Moreover, it will be great if someone can update uncrustify.cfg to our latest coding style guidelines. In my practice, uncrustify only get a proper new line before the namespace definition, but get other braces in {do, if, for, while} reverted into brace within single line.

glassez · 2024-12-16T14:58:16Z

Moreover, it will be great if someone can update uncrustify.cfg to our latest coding style guidelines.

I don't use it so I don't care. Let someone concerned take care of it.

glassez · 2024-12-16T15:00:11Z

@zent1n0
How about #21943 (comment)?

src/gui/rss/rsswidget.cpp

Co-authored-by: Vladimir Golovnev <[email protected]>

src/gui/rss/rsswidget.cpp

Chocobo1

last few comments about coding style (hopefully)

src/gui/rss/rsswidget.cpp

Co-authored-by: Chocobo1 <[email protected]>

Co-authored-by: Chocobo1 <[email protected]> Co-authored-by: Vladimir Golovnev <[email protected]>

zent1n0 · 2024-12-18T06:42:20Z

Should I squash the commits into one?

glassez · 2024-12-18T06:52:03Z

Should I squash the commits into one?

You can do it. Otherwise it will be done while merging PR.

zent1n0 · 2024-12-18T06:59:11Z

I prefer automatic methods 🤖 😊

zent1n0 marked this pull request as ready for review December 3, 2024 07:28

luzpaz added the RSS RSS-related issues/changes label Dec 3, 2024

glassez reviewed Dec 5, 2024

View reviewed changes

zent1n0 force-pushed the feat-abs-url branch from b3a16c9 to 2758bba Compare December 5, 2024 14:00

RSS widget: Convert relative paths to absolute

2cec901

In RSS widget, a relative url will cause an infinite loop loading for resource, which won't break until unselect the article explicitly format code

zent1n0 force-pushed the feat-abs-url branch from 2758bba to 2cec901 Compare December 5, 2024 14:00

glassez reviewed Dec 5, 2024

View reviewed changes

zent1n0 and others added 3 commits December 5, 2024 23:34

Update src/gui/rss/rsswidget.cpp

bfe3b98

Co-authored-by: Vladimir Golovnev <[email protected]>

Apply suggestions from code review

8c973b8

Co-authored-by: Vladimir Golovnev <[email protected]>

Move covert function into anonymous namespace

96cf6e8

Co-authored-by: Vladimir Golovnev <[email protected]>

zent1n0 force-pushed the feat-abs-url branch from 5fd9dd6 to 96cf6e8 Compare December 5, 2024 15:55

Chocobo1 reviewed Dec 6, 2024

View reviewed changes

Apply suggestions from code review

4d24395

Co-authored-by: Chocobo1 <[email protected]> Co-authored-by: Vladimir Golovnev <[email protected]>

zent1n0 force-pushed the feat-abs-url branch from 35abd80 to f254d0b Compare December 6, 2024 13:15

Correct terms and format

3d4efed

zent1n0 force-pushed the feat-abs-url branch from f254d0b to 3d4efed Compare December 6, 2024 13:20

zent1n0 changed the title ~~RSS widget: Convert relative paths to absolute~~ RSS widget: Convert relative URLs to absolute Dec 6, 2024

glassez changed the title ~~RSS widget: Convert relative URLs to absolute~~ Convert relative URLs to absolute within RSS article descriptions Dec 6, 2024

glassez changed the title ~~Convert relative URLs to absolute within RSS article descriptions~~ Resolve relative URLs within RSS article descriptions Dec 6, 2024

glassez changed the title ~~Resolve relative URLs within RSS article descriptions~~ Resolve relative URLs within RSS article description Dec 6, 2024

glassez reviewed Dec 6, 2024

View reviewed changes

Correct terms and move into anonymous namespace

c3dcba3

Format

f34c4ca

glassez reviewed Dec 16, 2024

View reviewed changes

src/gui/rss/rsswidget.cpp Outdated Show resolved Hide resolved

glassez reviewed Dec 16, 2024

View reviewed changes

src/gui/rss/rsswidget.cpp Show resolved Hide resolved

Apply suggestions from code review

ada90b5

Co-authored-by: Vladimir Golovnev <[email protected]>

fix: unintended modification to normalizedBaseUrl

e2cdcb2

glassez reviewed Dec 16, 2024

View reviewed changes

src/gui/rss/rsswidget.cpp Outdated Show resolved Hide resolved

glassez added this to the 5.1 milestone Dec 16, 2024

glassez self-assigned this Dec 16, 2024

Apply suggestions from code review

6f103cf

Co-authored-by: Vladimir Golovnev <[email protected]>

glassez previously approved these changes Dec 17, 2024

View reviewed changes

glassez requested a review from a team December 17, 2024 03:10

Chocobo1 reviewed Dec 17, 2024

View reviewed changes

src/gui/rss/rsswidget.cpp Outdated Show resolved Hide resolved

fix: incompatible operand types

e1c9bca

zent1n0 dismissed glassez’s stale review via e1c9bca December 17, 2024 19:03

Apply suggestions to move function ahead

3c37670

Chocobo1 reviewed Dec 17, 2024

View reviewed changes

src/gui/rss/rsswidget.cpp Outdated Show resolved Hide resolved

src/gui/rss/rsswidget.cpp Outdated Show resolved Hide resolved

src/gui/rss/rsswidget.cpp Outdated Show resolved Hide resolved

zent1n0 and others added 2 commits December 18, 2024 08:58

Apply suggestions from code review

9ca45e0

Co-authored-by: Chocobo1 <[email protected]>

Apply suggestions from code review

43a5547

Co-authored-by: Chocobo1 <[email protected]> Co-authored-by: Vladimir Golovnev <[email protected]>

glassez requested review from Chocobo1 and glassez December 18, 2024 06:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve relative URLs within RSS article description #21943

Resolve relative URLs within RSS article description #21943

zent1n0 commented Dec 3, 2024

glassez left a comment

glassez Dec 5, 2024

zent1n0 Dec 5, 2024

glassez Dec 5, 2024

glassez Dec 5, 2024

glassez Dec 5, 2024

zent1n0 commented Dec 5, 2024

glassez left a comment

glassez Dec 5, 2024

zent1n0 commented Dec 6, 2024

Chocobo1 left a comment

glassez commented Dec 6, 2024

glassez Dec 6, 2024

zent1n0 Dec 7, 2024

glassez Dec 6, 2024

zent1n0 Dec 7, 2024

zent1n0 commented Dec 7, 2024 •

edited

Loading

zent1n0 commented Dec 11, 2024

glassez commented Dec 11, 2024

zent1n0 commented Dec 12, 2024 •

edited

Loading

glassez commented Dec 14, 2024

zent1n0 commented Dec 14, 2024

glassez commented Dec 14, 2024

zent1n0 commented Dec 14, 2024 •

edited

Loading

glassez commented Dec 16, 2024

zent1n0 commented Dec 16, 2024

glassez commented Dec 16, 2024

glassez commented Dec 16, 2024

Chocobo1 left a comment

zent1n0 commented Dec 18, 2024

glassez commented Dec 18, 2024

zent1n0 commented Dec 18, 2024

	QString normalizeBasePath = basePath.endsWith(u'/') ? basePath : basePath + u'/';
	QString normalizedBasePath = basePath.endsWith(u'/') ? basePath : basePath + u'/';

Resolve relative URLs within RSS article description #21943

Are you sure you want to change the base?

Resolve relative URLs within RSS article description #21943

Conversation

zent1n0 commented Dec 3, 2024

glassez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zent1n0 commented Dec 5, 2024

glassez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zent1n0 commented Dec 6, 2024

Chocobo1 left a comment

Choose a reason for hiding this comment

glassez commented Dec 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zent1n0 commented Dec 7, 2024 • edited Loading

zent1n0 commented Dec 11, 2024

glassez commented Dec 11, 2024

zent1n0 commented Dec 12, 2024 • edited Loading

glassez commented Dec 14, 2024

zent1n0 commented Dec 14, 2024

glassez commented Dec 14, 2024

zent1n0 commented Dec 14, 2024 • edited Loading

glassez commented Dec 16, 2024

zent1n0 commented Dec 16, 2024

glassez commented Dec 16, 2024

glassez commented Dec 16, 2024

Chocobo1 left a comment

Choose a reason for hiding this comment

zent1n0 commented Dec 18, 2024

glassez commented Dec 18, 2024

zent1n0 commented Dec 18, 2024

zent1n0 commented Dec 7, 2024 •

edited

Loading

zent1n0 commented Dec 12, 2024 •

edited

Loading

zent1n0 commented Dec 14, 2024 •

edited

Loading